RSDK-11407: add streamServerMu to guard streamServer creation/deletion #5443
Conversation
viambot
added
the
safe to test
| func (svc *webService) closeStreamServer() { | ||
| svc.streamServerMu.Lock() | ||
| defer svc.streamServerMu.Unlock() | ||
| if err := svc.streamServer.Close(); err != nil { |
There was a problem hiding this comment.
Note: svc.streamServer.Close() acquires svc.streamServer.mu.
If I move the svc.streamServerMu.Lock()/Unlock() to line 37 instead and only guard the nil assignment, there will be another race on:
Read @ robot/web/stream.(*Server).refreshVideoSources(): server.robot
Write @ robot/web/stream.(*Server).Close(): server.activeBackgroundWorkers.Wait()
|
Seems reasonable to me. I defer to @cheukt who definitely knows the calls/lifetimes of these methods/objects better than myself. |
benjirewis
left a comment
benjirewis
left a comment
There was a problem hiding this comment.
I was hesitant to throw another mutex in here, but I do see how the one you've added does something that's different than streamServer.mu. Thanks! Just a thought on naming + documentation.
robot/web/web.go
Outdated
| isRunning bool | ||
| webWorkers sync.WaitGroup | ||
| modWorkers sync.WaitGroup | ||
| streamServerMu sync.Mutex |
There was a problem hiding this comment.
[nit] Could we call this streamServerInitCloseMu or something that helps differentiate from streamServer.mu? And maybe leave a comment above this mutex saying that it synchronizes the web service's concurrent access to the streamServer object?
viambot
added
safe to test
| defer svc.streamServerInitCloseMu.Unlock() | ||
| // May be nil if this is being run after closeStreamServer. | ||
| if svc.streamServer != nil { | ||
| return svc.streamServer.AddNewStreams(svc.cancelCtx) |
There was a problem hiding this comment.
should streamServer ever be nil if the web server is up? feels like that'd be bad since that would be we won't be able to get video streams
There was a problem hiding this comment.
a test would be to get the web server in such a state that the streamServer is nil during a reconfigure and see if we can still access webrtc video streams (UI Live camera streams, for example)
5 participants
See ticket for investigation findings.