eval broker: use write lock when reaping cancelable evals #16112
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
tgross
force-pushed
the
b-flaky-eval-broker-test
branch
from
1516fec
to
d85ee3c
Compare
tgross
force-pushed
the
b-flaky-eval-broker-test
branch
from
d85ee3c
to
e1b1df5
Compare
tgross
changed the title
tgross
force-pushed
the
b-flaky-eval-broker-test
branch
from
e1b1df5
to
78aea89
Compare
tgross
force-pushed
the
b-flaky-eval-broker-test
branch
from
78aea89
to
e462084
Compare
tgross
force-pushed
the
b-flaky-eval-broker-test
branch
from
8707739
to
f57c841
Compare
This was referenced Feb 10, 2023
Merged
Merged
The eval broker's `Cancelable` method used by the cancelable eval reaper mutates the slice of cancelable evals by removing a batch at a time from the slice. But this method unsafely uses a read lock despite this mutation. Under normal workloads this is likely to be safe but when the eval broker is under the heavy load this feature is intended to fix, we're likely to have a race condition. Switch this to a write lock, like the other locks that mutate the eval broker state. This changeset also adjusts the timeout to allow poorly-sized Actions runners more time to schedule the appropriate goroutines. The test has also been updated to use `shoenig/test/wait` so we can have sensible reporting of the results rather than just a timeout error when things go wrong.
tgross
force-pushed
the
b-flaky-eval-broker-test
branch
from
2268e8c
to
d76273d
Compare
tgross
changed the title
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.
The eval broker's
Cancelablemethod used by the cancelable eval reaper mutatesthe slice of cancelable evals by removing a batch at a time from the slice. But
this method unsafely uses a read lock despite this mutation. Under normal
workloads this is likely to be safe but when the eval broker is under the heavy
load this feature is intended to fix, we're likely to have a race
condition. Switch this to a write lock, like the other locks that mutate the
eval broker state.
This changeset also adjusts the timeout to allow poorly-sized Actions runners
more time to schedule the appropriate goroutines. The test has also been updated
to use
shoenig/test/waitso we can have sensible reporting of the resultsrather than just a timeout error when things go wrong.