RSDK-9316 Disallow client stream creation when channel has been closed #394
Conversation
|
Needs a test; adding one. |
| @@ -45,7 +50,14 @@ func newWebRTCClientStream( | |||
| stream *webrtcpb.Stream, | |||
| onDone func(id uint64), | |||
| logger utils.ZapCompatibleLogger, | |||
| ) *webrtcClientStream { | |||
| ) (*webrtcClientStream, error) { | |||
| // Assume that cancelation of the client channel's context means the peer | |||
There was a problem hiding this comment.
This is a good start to a comment. But I feel like there's a bit of "what's the consequence of removing this"? This just looks like an optimization to inform the caller sooner that their invocation/stream isn't going to work.
There was a problem hiding this comment.
Added some elaboration there; let me know what you think. Thanks for asking for more, will be helpful for posterity.
There was a problem hiding this comment.
Much better!
I'd also note (because it's very not obvious in this case) the caller is holding the channel mutex that's also acquired in the "close" code path that's canceling this context.
There was a problem hiding this comment.
For sure; included (almost) exactly what you said there.
viambot
added
safe to test
viambot
added
safe to test
RSDK-9316
Checks the underlying channel's context for error before creating a new client stream. Returns a new
ErrDisconnectederror if the context has errored. A PR like this one would makerdksensitive to that new error instead of just waiting forio.ErrClosedPipeafter an eventual read/write (?) from/to the closed channel.Confirmed that this PR plus the linked
rdkone gets rid of the race seen in viamrobotics/rdk#4566 CI.