-
-
Notifications
You must be signed in to change notification settings - Fork 593
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 TestAkamaiPurgerDrainQueueSucceeds data race #7389
Fix TestAkamaiPurgerDrainQueueSucceeds data race #7389
Conversation
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.
Thanks for working on fixing this! Looking at the data race stack trace in #7388 (thanks for that), it appears the problem is that purgeBatch is reading, without synchronization, from the same memory that is written to by incoming gRPC connections (in Purge
, line 230)
But that's weird because the unsynchronize read, on main.go line 184, is reading only local variables and function parameters.
The problem is that the function parameter batch
is a slice, in other words a pointer to some backing array. And that slice originates in takeBatch, where the access is synchronized.
By my read of the code, the expectation is that the slicing operation in takeBatch line 214 creates a copy of the underlying data, so no synchronization is needed to access the returned information. That's wrong, though; re-slicing a slice keeps pointing to the same underlying data.
The fix that would be most in keeping with the style of the code as written would be to have takeBatch
actually return a deep copy of the relevant slice. Unfortunately, since it returns a slice of slices, slices.Clone
doesn't suffice. I think this needs a for
loop, though each iteration of the loop could do a slices.Clone
of the three-element inner slice (containing the three forms of URL to be purged).
@jsha the fix now uses a deep copy of the purge entries rather than an additional lock. |
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.
Looks great. You'll need to wait for one more review and approval before we can merge. Thanks a lot for working on this!
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.
Thanks!
Fixes #7388