Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean up resources in Go channel pub sub #407

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

avlajcic-axilis
Copy link

Issue

Part of the issue reported here #391
After testing, I've found same issue in Publish method. It uses LoadOrStore for fetching subLock.

subLock, _ := g.subscribersByTopicLock.LoadOrStore(topic, &sync.Mutex{})

That means if lock for specified topic doesn't exist it will create a new one. Because that data doesn't get removed, it means it will fill up map with new data even if there are no subscribers for this topic. If we use dynamic name of topics map size will continue to increase over time.

With fix in #396 data will only get removed if subscriber disconnects from specific topic. If no subscribers connect at any point, data will never get removed.

Fix

For Publish method, I've added condition to check if new lock has been loaded or added, and if it's added remove it at the end.
There is an option to use Load instead of LoadOrStore and do early return here because if lock doesn't exist, it means there are no subscribers to send data to. I did't want to change much of the logic, but let me know if you think that is something that could be done.
Only difference would be is that we would need to move handling of persistent message before that check.

For Subscribe method, fix is introduced in #396 which is included is this PR.

Tests

Since these fixes are related to internal fields, I've added new test file with the same package name so we can test those fields.
First test asserts that all data from subscriber maps has been removed after subscribers disconnect.
Second tests asserts that no data is left over in subscribers lock map after publishing messages. Lock for single subscriber should still exist in map.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants