drpcmanager: derive shutdown waiting from active streams - #82
Open
shubhamdhama wants to merge 1 commit into
Open
drpcmanager: derive shutdown waiting from active streams#82shubhamdhama wants to merge 1 commit into
shubhamdhama wants to merge 1 commit into
Conversation
shubhamdhama
requested review from
roachdev-claude
and removed request for
roachdev-claude
July 27, 2026 07:33
Nukitt
approved these changes
Jul 28, 2026
| // Manager.NewClientStream (newStream → create stream → add to streams). The | ||
| // activeStreams lock ensures a stream is either registered before Close starts | ||
| // waiting for the collection to drain or rejected after shutdown begins. | ||
| func TestManager_ConcurrentCloseAndNewClientStream(t *testing.T) { |
There was a problem hiding this comment.
not a blocker- do we also want a test that can force a different goroutine to win terminate? will have better coverage.
suj-krishnan
approved these changes
Jul 28, 2026
suj-krishnan
left a comment
suj-krishnan
left a comment
There was a problem hiding this comment.
LGTM! This is definitely more elegant than what we have today. Please check the nit and the test-gaps.
| // Manager.NewClientStream (newStream → create stream, add to streams, wg.Add). | ||
| // Under -race this would fail without wg.Add being atomic with the closed | ||
| // check in activeStreams.Add. | ||
| // Manager.NewClientStream (newStream → create stream → add to streams). The |
There was a problem hiding this comment.
We can add tests around a few scenarios that we have explicitly guarded against (please ignore if the existin tests already cover these).
- Two active streams: remove only the first, verify that Wait does not unblock
- Test that Wait on an empty but open registry blocks until Close.
- Remove streams before Close and verify that Close wakes an existing waiter.
Each manageStream goroutine corresponds to one activeStreams entry. Keeping a separate WaitGroup duplicated that lifecycle state and made activeStreams.Add update synchronization owned by Manager. Keep canceled streams registered until their manage goroutines remove them. Once shutdown starts and the registry is empty, close a completion channel so Manager.Close can wait on the registry itself. The registry mutex keeps stream admission atomic with shutdown. Use the same mutex to guard the one-time channel close. Cover both admission orderings and verify that Wait blocks until Close and all registered streams are removed. Co-authored-by: Cursor <cursoragent@cursor.com>
shubhamdhama
force-pushed
the
remove-wg-from-manager
branch
from
July 29, 2026 07:56
aace5c2 to
7404483
Compare
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Each manageStream goroutine corresponds to one activeStreams entry.
Keeping a separate WaitGroup duplicated that lifecycle state and made
activeStreams.Add update synchronization owned by Manager.
Keep canceled streams registered until their manage goroutines remove
them. Once shutdown starts and the registry is empty, close a completion
channel so Manager.Close can wait on the registry itself. The registry
mutex keeps stream admission atomic with shutdown.
Use the same mutex to guard the one-time channel close. Cover both
admission orderings and verify that Wait blocks until Close and all
registered streams are removed.