-
Notifications
You must be signed in to change notification settings - Fork 73
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
fix panic when under pressure #3098
Conversation
84783f6
to
52bc8cf
Compare
} | ||
|
||
const enqueueTimeout = 500 * time.Millisecond | ||
const enqueueTimeout = 5 * time.Minute |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line owes me two days of life and one year of therapy. 🙃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small thing but it can panic the server
subscribers := append(m.getSubscribers(resourceID), subscriber) | ||
m.setSubscribers(resourceID, subscribers) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't you need to lock the mutex here to prevent read-write race conditions here? Go will panic in case of read-write concurrency: https://stackoverflow.com/questions/36167200/how-safe-are-golang-maps-for-concurrent-read-write-operations
Same for PublishUpdate
Edit: reproduced the issue here (you might have to run it multiple times to see it fail): https://go.dev/play/p/h_AIcsi2J-Z
fatal error: concurrent map read and map write
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Originally each function that needed to access the map would manage the lock itself. That code was causing issues for some reason, so I moved the usage of the lock to the getSubscribers function and the setSubscribers function, so effectively every read and write is lock protected
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm
This PR fixes the performace issues detected by running k6 tests
Changes
Fixes
Checklist