kvcoord: Use locked map in mux rangefeed #99289
Conversation
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
There was a problem hiding this comment.
Looks good to me, thanks! I didn't review the first commit - it doesn't seem like it should be in this PR. Please remove it before merge (you can send it separately) or indicate that it was included on purpose, in which case we should update the PR description and I'll review the commit.
| // gets properly recorded even if mux go routine terminates right after the | ||
| // above sender.Send() succeeded. | ||
| c.streams.Store(streamID, unsafe.Pointer(stream)) | ||
| c.mu.streams[streamID] = stream |
There was a problem hiding this comment.
Could you add something about the race here, like:
// Note that since we already called
Send(req), events from this stream might be showing up in the processing goroutines, and they need to be able to look up the stream. Since we still hold the mutex (and held it acrossSend), this will hold true. See: #97817.
Yup -- it was merged elsewhere. |
Use locked map instead of IntMap to ensure correct synchronization between stream start up and termination. Fixes cockroachdb#99096 Release note: None Release justficiation: bug fix
|
bors r+ |
1 similar comment
|
bors r+ |
|
Build succeeded: |
99469: release-23.1: kvcoord: Use locked map in mux rangefeed r=miretskiy a=blathers-crl[bot] Backport 1/1 commits from #99289 on behalf of `@miretskiy.` /cc `@cockroachdb/release` ---- Use locked map instead of IntMap to ensure correct synchronization between stream start up and termination. Fixes #99096 Release note: None Release justification: bug fix ---- Release justification: Co-authored-by: Yevgeniy Miretskiy <[email protected]>
Use locked map instead of IntMap to ensure correct
synchronization between stream start up and termination.
Fixes #99096
Release note: None
Release justification: bug fix