Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.
Sign upGraceful shutdown #108
Conversation
| @@ -476,6 +486,7 @@ func (wp *wsPeer) Close() { | |||
| atomic.StoreInt32(&wp.didSignalClose, 1) | |||
| if atomic.CompareAndSwapInt32(&wp.didInnerClose, 0, 1) { | |||
| close(wp.closing) | |||
| wp.conn.WriteControl(websocket.CloseMessage, websocket.FormatCloseMessage(websocket.CloseNormalClosure, ""), time.Now().Add(5*time.Second)) | |||
This comment has been minimized.
This comment has been minimized.
tsachiherman
Jul 1, 2019
•
Contributor
We need to have this function receive a context, and to have the WriteControl respect that ( somehow ).
We don't want to have a "stuck" endpoint to hang our shutdown process.
sorry - I know it's not easy to implement ;-(
This comment has been minimized.
This comment has been minimized.
algobolson
Jul 1, 2019
Author
Collaborator
It has a deadline (now + 5 seconds). How this is called in wsNetwork.go will start a thread per peer, so they'll all wait those 5 seconds in parallel and so at most 5 seconds after WebsocketNetwork.Close() is called it should be done.
I agree in principle, it would be neat to upgrade all of websocket to Write and Read with context. Can we punt for a separate soonish TODO issue?
This comment has been minimized.
This comment has been minimized.
tsachiherman
Jul 1, 2019
Contributor
of course. This change is a great improvement on its own and shouldn't be deferred.
We can make it even better tomorrow ;-)
|
LGTM |
|
Want to confirm the answer to my question before this gets checked in |
algobolson commentedJul 1, 2019
Summary
Eliminate warning message when a client disconnects normally.
Test Plan
I made a private 3 node network. I watched the logs on the relay node while
killorkill -9of a leaf node.kill -9of a node caused a warning message that a node went away unexpectedly, but regularkillexited the node gracefully such that it sent a websocket CloseMessage and the relay node logged no warning.