Server now closes socket when connection is dropped by client ( Fix #19 ) #20
Conversation
…d sockets hanging in CLOSE_WAIT
Codecov Report
@@ Coverage Diff @@
## main #20 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 2 2
Lines 195 200 +5
Branches 41 42 +1
=========================================
+ Hits 195 200 +5
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
src/simple_websocket/ws.py
Outdated
| @@ -163,11 +163,15 @@ def _thread(self): | |||
| if len(in_data) == 0: | |||
| raise OSError() | |||
| except (OSError, ConnectionResetError): # pragma: no cover | |||
| self.close(reason=CloseReason.GOING_AWAY, | |||
| message='Connection Error') | |||
There was a problem hiding this comment.
Maybe there is some corner case that I'm missing, but isn't this exception handler being invoked when the other side goes away? So what would be the point to send a close packet to that other side that is already gone?
There was a problem hiding this comment.
I left it there because I couldn't be sure that an OSError was caused only by the client going away, it could be something on our side (system issues), for example problems in the recv(). Replacing it with a self.sock.close() would work, however we may incur in protocol violation if it's not true that the other side went away, I reckon.
There was a problem hiding this comment.
Reanimating this: Should I change this to self.sock.close()? @miguelgrinberg
src/simple_websocket/ws.py
Outdated
| @@ -348,6 +352,10 @@ def choose_subprotocol(self, request): | |||
| return subprotocol | |||
| return None | |||
|
|
|||
| def close(self, reason=None, message=None): | |||
| super().close(reason=reason, message=message) | |||
| self.sock.close() | |||
There was a problem hiding this comment.
Is this the correct way to handle a graceful connection end? It seems the side that is going away sends a close packet, but does not give the other side to acknowledge and respond, since it immediately closes the socket as well. I think the correct closing handshake is for the first party to send the close packet, then wait for the other side to response with their own close packet, and only then both close their sockets.
And if we assume this is correct, why is only the server implementation closing the socket, but not the client?
There was a problem hiding this comment.
I think you're right, we shouldn't force the socket to close right away, I overlooked that part of the websocket protocol. However, the Client, as far as I can understand, is in fact closing the socket after super().close() (line 440), actually it was bit of "cargo cult" from my part using that pattern on Server.
I can definitely remove this override of close() if you agree it's the best way and test again. However I'm not comfortable changing Client.close() as I'm not confident in my ability to test it fully.
There was a problem hiding this comment.
Reanimating this: Do you agree on removing this on close() of Server?
There was a problem hiding this comment.
Yes, as long as the close call happens when the server receives the close confirmation packet from the client. I think that should be already in place, but should probably be verified.
There was a problem hiding this comment.
Thank you, I'll change it, analyse the code to check for the close on the Server and retest the original problematic case again
…socket disconnection after send a close packet.
|
This is now merged. I have simplified the closing logic a bit, and also added a missing close for the selector, which is used when the optional pings are enabled. Thanks! |
|
Thanks for your work! |
Server now closes socket when connection is dropped by client to avoid sockets hanging in CLOSE_WAIT state.
This aims at solving issue #19 as discussed. I changed the solution a bit from what was proposed at first to follow the pattern already existent in Client. I'm not sure however about the best reason and message for the disconnection.
I'm not sure about the testing rules for this branch, I tested this case has best I could manually. If I should do something else, like updating automatic tests, please do say.
Also took the liberty to fix a couple of typos in comments, hope you don't mind.
Thank you