Re-enable ping timeout behaviour for 3x branch #351
Conversation
For some reason the ping timeout was removed from V3 of the cpp client library.
| { | ||
| if (m_ping_timeout_timer) { | ||
| asio::error_code ec; | ||
| m_ping_timeout_timer->expires_from_now(milliseconds(m_ping_timeout),ec); | ||
| m_ping_timeout_timer->async_wait(std::bind(&client_impl::timeout_pong, this, std::placeholders::_1)); | ||
| } | ||
| // Parse the incoming message according to socket.IO rules |
There was a problem hiding this comment.
Shouldn't you call update_ping_timeout_timer() here?
| { | |
| if (m_ping_timeout_timer) { | |
| asio::error_code ec; | |
| m_ping_timeout_timer->expires_from_now(milliseconds(m_ping_timeout),ec); | |
| m_ping_timeout_timer->async_wait(std::bind(&client_impl::timeout_pong, this, std::placeholders::_1)); | |
| } | |
| // Parse the incoming message according to socket.IO rules | |
| { | |
| update_ping_timeout_timer() | |
| // Parse the incoming message according to socket.IO rules |
There was a problem hiding this comment.
I could re-add this, but I'm not sure it's "correct" behaviour to essentially treat every received message from the server as a "ping". I have checked the client implementation for Java script, and it seems like that client only updates the timer on a ping package also.
https://github.com/socketio/socket.io-client/blob/master/dist/socket.io.js
There was a problem hiding this comment.
You are right. The server is the one periodically sending ping packets that the client must reply. For more info see: https://socket.io/docs/v4/how-it-works/#disconnection-detection
|
Hi @TorMFinn thanks for your contribution! If you could answer my question and change it (if necessary) then this PR will be ready to go 😊 |
|
Hi @jmigual I have commented on your question :) |
After this commit ec4d540 the ping timeout behavior was broken.
Fixes: