*: use per-stream Finished signal instead of shared sfin channel#45
*: use per-stream Finished signal instead of shared sfin channel#45shubhamdhama wants to merge 1 commit intoshubham/simplify-manageStreamsfrom
Conversation
shubhamdhama
force-pushed
the
shubham/simplify-manageStreams
branch
from
2b7433e to
cf11131
Compare
shubhamdhama
force-pushed
the
shubham/use-stream-finished
branch
from
779b184 to
341397a
Compare
Replace the shared sfin channel with stream.Finished(), giving each stream its own completion signal. The shared channel worked for single-stream-at-a-time. Per-stream signals are required for multiplexing where multiple streams finish independently.
shubhamdhama
force-pushed
the
shubham/simplify-manageStreams
branch
from
cf11131 to
b7ec558
Compare
shubhamdhama
force-pushed
the
shubham/use-stream-finished
branch
from
341397a to
dcda07e
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates stream completion signaling to use the per-stream (*drpcstream.Stream).Finished() channel rather than a single shared “stream finished” channel, which is necessary for correct behavior when multiple streams can complete independently (multiplexing).
Changes:
- Removed the shared
sfinchannel fromdrpcmanager.Managerand switched waiting logic tostream.Finished(). - Removed
finplumbing fromdrpcstream.Streamconstruction and finish-path signaling. - Deleted
GetStreamFin/SetStreamFinfrom internal stream options.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| internal/drpcopts/stream.go | Removes accessors for the old shared finish channel in internal stream options. |
| drpcstream/stream.go | Removes the stream-level fin channel and stops emitting to a shared finished notifier. |
| drpcmanager/manager.go | Replaces waits on the shared sfin channel with waits on stream.Finished(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // GetStreamTransport returns the drpc.Transport stored in the options. | ||
| func GetStreamTransport(opts *Stream) drpc.Transport { return opts.transport } | ||
|
|
||
| // SetStreamTransport sets the drpc.Transport stored in the options. | ||
| func SetStreamTransport(opts *Stream, tr drpc.Transport) { opts.transport = tr } | ||
|
|
||
| // GetStreamFin returns the chan<- struct{} stored in the options. | ||
| func GetStreamFin(opts *Stream) chan<- struct{} { return opts.fin } | ||
|
|
||
| // SetStreamFin sets the chan<- struct{} stored in the options. | ||
| func SetStreamFin(opts *Stream, fin chan<- struct{}) { opts.fin = fin } | ||
|
|
||
| // GetStreamKind returns the StreamKind stored in the options. | ||
| func GetStreamKind(opts *Stream) drpc.StreamKind { return opts.kind } | ||
|
|
There was a problem hiding this comment.
After removing GetStreamFin/SetStreamFin, the underlying fin chan<- struct{} field still remains in drpcopts.Stream (see internal/drpcopts/stream.go:14) but is now unreachable outside the package and has no remaining references in the repo. Consider deleting the fin field from drpcopts.Stream to avoid dead/unused internal state and confusion about how stream completion is signaled (now via (*drpcstream.Stream).Finished()).
|
I don't believe this change is needed in mux world? The only reason |
Replace the shared sfin channel with stream.Finished(), giving each
stream its own completion signal. The shared channel worked for
single-stream-at-a-time. Per-stream signals are required for
multiplexing where multiple streams finish independently.