ca: Readd WaitGroup to Server #1854
Conversation
The WaitGroup was removed as a simplification, but this causes (*Server).Stop to potentially return before Run is finished. This is unwanted behavior that causes data races in tests. Readd the WaitGroup, but only use it to wait for Run to finish, not RPC handlers. Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>
| @@ -464,7 +467,12 @@ func (s *Server) Run(ctx context.Context) error { | |||
| // Stop stops the CA and closes all grpc streams. | |||
| func (s *Server) Stop() error { | |||
| s.mu.Lock() | |||
|
|
|||
| // Wait for Run to complete before returning | |||
| defer s.wg.Wait() | |||
There was a problem hiding this comment.
Should this be called after the check for !s.isRunning()?
There was a problem hiding this comment.
Alternately, maybe only call wait if s.isRunning(), since otherwise won't s.isRunning() always be false?
There was a problem hiding this comment.
I want it to wait in that case as well, because Run may not have picked up the context cancellation yet.
There was a problem hiding this comment.
It may not matter, since I don't think we're checking the error returned by Stop, but would this mean that Stop() always returns an error?
There was a problem hiding this comment.
Also, s.cancel() seems to be called after the check for !s.isRunning() - would this just block unless something else cancelled the Run context?
There was a problem hiding this comment.
Nm, for some reason my eyes kept slipping passed the word defer
|
LGTM |
|
Merging this to fix CI on master. Feel free to do more review after the fact. |
The
WaitGroupwas removed as a simplification in #1841, but this causes(*Server).Stopto potentially return before Run is finished. This is unwanted behavior that causes data races in tests. Readd theWaitGroup, but only use it to wait for Run to finish, not RPC handlers.cc @dperny @cyli