search

skaarj1989 / mWebSockets

WebSockets for microcontrollers
https://skaarj1989.github.io/mWebSockets/autobahn-testsuite/servers/
MIT License
108 stars 23 forks source link

Removed WebSocket::read() timeout #31

Closed prenone closed 3 years ago

prenone commented 3 years ago

I have removed the timeout from the WebSocket::_read() function.

This function gets called by WebSocketClient::listen(). I think the desired effect is to terminate the connection if nothing is received within kTimeoutInterval milliseconds. However I can't find anything regarding such timeout in RFC6455 and the only thing that it does in my experience is slow down the overall execution and unexpectadly close the connection. This is because WebSocketClient::listen() purpose is to be called once per loop() checking for new data and reading that data only if it actually exists. There is no specification that guarantees data within a time interval.

IMHO my chages are correct, however I see how this could break some applications that rely on this behaviour. The correct way to implement a timeout is with the WebSocket Ping or with any custom ping on top. Maybe this change should be implemented with a major realease or someway to toggle it in config.h?

Removing the timeout from WebSocket::_read() does not break the library (I have production code that has been running for weeks without the timeout).

Btw thanks for the amazing library :smile:

prenone commented 3 years ago

As with the last pull request I should have checked the issues before 🤣. This seems to be to related to #29. Sorry, these are some busy times.

skaarj1989 commented 3 years ago

Hi @prenone I think that this piece of code comes from times before I implemented ping/pong. Tomorrow I will run autobahn tests.

skaarj1989 commented 3 years ago

@prenone Unfortunately some tests failed. See attachment if you are curious.

prenone commented 3 years ago

What is the procedure to perform the Autobahn test? Is there a proper sketch I don't see?

skaarj1989 commented 3 years ago

Testsuite is located in dev branch You will need Docker to perform them.

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.