[bugfix] Lock when checking/creating notifs to avoid race #2890
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tsmethurst
commented
May 2, 2024
| @@ -395,9 +414,24 @@ func (s *surface) notify( | |||
| return nil | |||
| } | |||
|
|
|||
| // We're doing state-y stuff so get a | |||
| // lock on this combo of notif params. | |||
| lockURI := getNotifyLockURI( | |||
There was a problem hiding this comment.
The actual fix is here.
tsmethurst
commented
May 2, 2024
| WorkersTestSuite | ||
| } | ||
|
|
||
| func (suite *SurfaceNotifyTestSuite) TestSpamNotifs() { |
There was a problem hiding this comment.
And here's the test for the fix.
nyarla
pushed a commit
to nyarla/gotosocial-modded
that referenced
this pull request
Jun 19, 2024
…usbusiness#2890) * [bugfix] Lock when checking/creating notifs to avoid race * test notif spam
nyarla
pushed a commit
to nyarla/gotosocial-modded
that referenced
this pull request
Jun 19, 2024
…usbusiness#2890) * [bugfix] Lock when checking/creating notifs to avoid race * test notif spam
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This pull request adds some locks in the surfaceNotify functionality to ensure that multiple notifications aren't created for the same status. Previously it was possible to create multiple notifs if the function was called in parallel (see added tests, which fail without the fix).
Should close #2858
Note: to test this in isolation I had to expose a few things in the surface package, and I renamed the AccountLocks thing to ProcessingLocks too. The actual fix itself is very small.
Here's how it looks now when you spam
surface.Notify20 times, in the format[timestamp] - [goroutine nr.] [message]In this scenario, goroutine 19 is the one that actually creates the notif, and the rest just see if it exists and leave.
Here's the same but without the lock fix:
So in this scenario the notif gets created a bunch of times.
Checklist
Please put an x inside each checkbox to indicate that you've read and followed it:
[ ]->[x]If this is a documentation change, only the first checkbox must be filled (you can delete the others if you want).
go fmt ./...andgolangci-lint run.