[FIXED] #4789 to allow a restart of a shutdown server instance. #4792
Conversation
Signed-off-by: Derek Collison <derek@nats.io>
| @@ -2083,6 +2083,11 @@ func (s *Server) Start() { | |||
|
|
|||
| defer s.Noticef("Server is ready") | |||
|
|
|||
| // Check is this may be a restart. | |||
| if s.startupComplete == nil { | |||
There was a problem hiding this comment.
I think one should ask if we want to support calling s.Start() after a s.Shutdown(). If yes, then I think it is a bit more than that, that needs to be done. I believe that there are others things that are created/setup in NewServer() whose state may be changed during Shutdown() and that Start() will not take into account.
| s.Shutdown() | ||
| s.WaitForShutdown() | ||
| s.Start() | ||
| defer s.Shutdown() |
There was a problem hiding this comment.
Case in point, would s.Shutdown() not panic because channel shutdownComplete has already been closed in previous Shutdown()? My point is that I believe that there way more state that would need to be recreated/reset on a Start() after a Shutdown() that we can think of.
There was a problem hiding this comment.
If it does not panic, it means that the second Shutdown() is actually not shutting down, likely because the code thinks it is already shutdown...
There was a problem hiding this comment.
Meaning we should expand the test to make sure the server is usable? Or are you suggesting that the second start should fail and you can not re-use servers that have been shutdown?
There was a problem hiding this comment.
The latter. I think that there is a lot of "state" that would need to be recreated/reset if we wanted to support a Start() after a Shutdown() (and therefore subsequent Shutdown() calls..). The only issue I have with that is Start() does not return an error, so that would leave the user not knowing that it is not started again. We could add a StartServer() or something like that, that would return an error (similar to what we did with NewServer vs New).
Resolves: #4789
Signed-off-by: Derek Collison derek@nats.io