crossbario / autobahn-python

WebSocket and WAMP in Python for Twisted and asyncio
https://crossbar.io/autobahn
MIT License
2.47k stars 763 forks source link

Allign websocket sending close codes with RFC #1642

Open soerenbe opened 4 weeks ago

soerenbe commented 4 weeks ago

Hi everyone, I use autobahn with daphne/Django for a websocket application. We got a usecase where we disconnect the client and reset the connection. To show this in the log files and properly us another reconnect strategy, we decided to use different websocket close codes. (e.g. 1012; Service Restart). We realized that this codes are not supported by daphne (https://github.com/django/daphne/issues/374). After looking around in the code I realized that this is implemented in the in this project.

Code: https://github.com/crossbario/autobahn-python/blob/master/autobahn/websocket/protocol.py#L2054

I am not sure why not all close codes are supported. Even the linked document in the code (https://www.iana.org/assignments/websocket/websocket.xml#close-code-number-rules) list all close codes.

Is there any reason why the close codes 1001-2999 are not supported? I think at least the defined codes 1000-1015 should be supported like described in the document.

I would create a MR, but the code look like this is intentional. :-)

Kind Regards Sören

oberstet commented 3 weeks ago

Autobahn is supposed to support all valid WebSocket standard close codes, and I believe the code is written

https://github.com/crossbario/autobahn-python/blob/f38f16ba28fa253dee951068cc729089c88d857d/autobahn/websocket/protocol.py#L441

and tested accordingly

https://github.com/crossbario/autobahn-testsuite/blob/master/autobahntestsuite/autobahntestsuite/case/case7_7_X.py

soerenbe commented 3 weeks ago

Hi @oberstet,. thanks for point this out. I think I have to dive a little deeper anf figure out whats the difference between the condition/error I ran into ( https://github.com/crossbario/autobahn-python/blob/master/autobahn/websocket/protocol.py#L2054) and the line you pointed out (gets checked at https://github.com/crossbario/autobahn-python/blob/f38f16ba28fa253dee951068cc729089c88d857d/autobahn/websocket/protocol.py#L721)

From the first look i might be the difference, that I want to SEND the close code and your code is for RECEIVING close codes.

I will look into that on Monday.

soerenbe commented 3 weeks ago

Hi @oberstet, I just explored the project testsuite and autobahntestsuite project. The second one was kinda overwhelming to me, but luckily I can describe the problem with the project testsuite.

I think my first look was right. You where pointing at receiving close codes and I had problems with sending close codes.

I am also was able to specify a "minimal" Testcase. I took the test test_sendClose_invalid_code_value from here: https://github.com/crossbario/autobahn-python/blob/f38f16ba28fa253dee951068cc729089c88d857d/autobahn/websocket/test/test_websocket_protocol.py#L217 and changed the parameter of the sendClose method from 10 to 1012.

I would say, that following test must success (but it doesnt)

    def test_sendClose_invalid_code_value_1012(self):
        self.protocol.sendClose(code=1012)

Also the interface description states that the close code must be "1000 for normal close or 3000-4999 for application specific close": https://github.com/crossbario/autobahn-python/blob/f38f16ba28fa253dee951068cc729089c88d857d/autobahn/websocket/interfaces.py#L547

As described above this matches with the implementation in https://github.com/crossbario/autobahn-python/blob/f38f16ba28fa253dee951068cc729089c88d857d/autobahn/websocket/protocol.py#L2054, but I would argue that there are missing close codes.

Ich would also say, that to list of acceptable close codes for sending and receiving should be the same. As pointed out by you, they are already listed in

https://github.com/crossbario/autobahn-python/blob/f38f16ba28fa253dee951068cc729089c88d857d/autobahn/websocket/protocol.py#L492

and checked at

https://github.com/crossbario/autobahn-python/blob/f38f16ba28fa253dee951068cc729089c88d857d/autobahn/websocket/protocol.py#L721

I changed the topic name to match my description.

I think I now have everything together, I have some residual doubts since this is the first time I look that close into this implementation.