Fix panic on Shutdown when there is at least one active connection #1
Conversation
| @@ -128,7 +138,7 @@ func (s *Server) Restart() { | |||
|
|
|||
| // Shutdown performs a graceful server shutdown. | |||
| func (s *Server) Shutdown() { | |||
| s.shutdown <- true | |||
| close(s.shutdown) | |||
There was a problem hiding this comment.
Why does this need to change? If no message is sent to the shutdown channel, how will the listeners know the shutdown has occurred? (e.g. ln.80)
There was a problem hiding this comment.
Point №1: Actually, we never need the value sent to the shutdown channel. Actually, we can change the type of this channel from chan bool to chan struct{}.
Point №2: When we close the channel, then all the readers from this channel (<-s.shutdown) receive the default value (in this case false value).
Conclusion: closing the shutdown channel will work as a mechanism of informing all the goroutines about the sse server shutdown process.
There was a problem hiding this comment.
Ah oh, I am not experienced in using channels and I didn't know the default value got sent when a channel is closed.
| @@ -255,7 +265,6 @@ func (s *Server) dispatch() { | |||
| close(s.addClient) | |||
| close(s.removeClient) | |||
| close(s.closeChannel) | |||
| close(s.shutdown) | |||
There was a problem hiding this comment.
Why did this move to shutdown function? The existing behaviour made sense to me.
- Shutdown function is called which sends an event to the shudown channel
- dispatch function receives the shutdown event and then closes the channel as no further events are expected
There was a problem hiding this comment.
As you can see, in this PR all the serving goroutines (ServerHTTP) use reading from the s.shutdown channel. At the same time, thedispatch function also reads from this channel. It means, that if we just send some value to this channel, we don't know where this value will be read: in the dispatch function or in the one of running ServeHTTP functions. If we close this channel, then all the functions will receive default values by reading from this channel.
There was a problem hiding this comment.
Thank you for the explanation, that makes sense now :)
This is the fix for the "panic on sse-server shutdown when there is at least one active connection" issue.
It does the following: 1) after calling the Shutdown() function it doesn't allow new sse-connections to be established 2) it avoids sending to already closed removeClient channel