vsr: fix races in client_replies #1515
Merged
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
matklad
force-pushed
the
matklad/raced2
branch
from
5bdd222
to
5f1a140
Compare
|
Alternative fix is available at #1513, though I like this version more. |
matklad
force-pushed
the
matklad/raced2
branch
from
5f1a140
to
0ba9044
Compare
sentientwaffle
previously approved these changes
Feb 5, 2024
src/vsr/client_replies.zig
Outdated
| @@ -243,6 +243,15 @@ pub fn ClientRepliesType(comptime Storage: type) type { | |||
| client_replies.write_reply_next(); | |||
| } | |||
|
|
|||
| if (callback == null) { | |||
There was a problem hiding this comment.
Could rewrite this into const callback = read.callback orelse { ... to avoid callback.? later.
There was a problem hiding this comment.
This needs to be a bit more complicated as the read's released here already, but, on balance, yeah, this seems better.
| while (reads.next()) |read| { | ||
| if (read.slot.index == write.slot.index) return; | ||
| } | ||
|
|
There was a problem hiding this comment.
Maybe in write_reply_callback we could iterate the reads and assert that none of them are to the slot that we just wrote?
There was a problem hiding this comment.
There actually can be races after the state sync.
TIL: in our testing storage, we forbid write-write races, but allow write-read races. Which ... seems fine? At least in this case.
matklad
force-pushed
the
matklad/raced2
branch
2 times, most recently
from
f464c4e
to
9db2919
Compare
- it might be the case that `writing` bitset is not set, but there's a
write for the slot in the queue: replies can wait on read to complete.
- when a faulty read completes, it might clobber faulty bit, unset by a
a write which was scheduled after the read.
The specific series of events here:
1. Replica receives a RequestReply and starts a reply read
2. The read completes with a failure, replica sets the faulty bit
3. Replica receives RequestReply starts a reply read
4. Replica receives Reply and starts a reply write
- the write unsets faulty bit
- the write doesn't start, because there's a read executing
4. The read completes, setting the faulty bit _again_
5. Replica receives RequestReply
- It _doesn't_ start reply read, because there's an in-progress write
that can resolve a read.
- But the faulty bit is set, tripping up an assertion.
The root issue here is the race between a read and a write for the same
reply. Remove the race by explicitly handling the interleaving:
* When submitting a read, resolve it immediately if there's a pending
write (this was already handled by `read_reply_sync`)
* When submitting a write, resolve any pending reads for the same reply.
* Remove the code to block the write while the read is in-progress, as
this is no longer possible.
Note that it is still possible that a read and a write for the same slot
race, if they target different replies. In this case, there won't be
clobbering, as, when the read completes, we double-check freshness by
consulting `client_sessions`.
SEED: 2517747396662708227
Closes: #1511
matklad
force-pushed
the
matklad/raced2
branch
from
9db2919
to
4db067f
Compare
sentientwaffle
approved these changes
Feb 5, 2024
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.
writingbitset is not set, but there's a write for the slot in the queue: replies can wait on read to complete.The specific series of events here:
The root issue here is the race between a read and a write for the same reply. Remove the race by explicitly handling the interleaving:
read_reply_sync)Note that it is still possible that a read and a write for the same slot race, if they target different replies. In this case, there won't be clobbering, as, when the read completes, we double-check freshness by consulting
client_sessions.SEED: 2517747396662708227
Closes: #1511