socket.io icon indicating copy to clipboard operation
socket.io copied to clipboard

Published 20 hours ago • verified publisher icon socketio

fix: Check if missed heartbeat synchronously before sending message

Open tjenkinson opened this issue 1 year ago • 1 comments

Chrome (and maybe other browsers) will throttle setTimeout under certain conditions, causing them to slow down, and this breaks the heartbeat detection logic.

This fix means if a message is emitted when the connection should be considered expired (but the setTimeout hasn't fired yet), we close that socket and then buffer the message for the new connection. So now a message should never be sent over an expired connection.

This doesn't solve the problem of connections being detected as expired too late. I think that could be done separately.

The kind of change this PR does introduce

  • [x] a bug fix
  • [ ] a new feature
  • [ ] an update to the documentation
  • [ ] a code change that improves performance
  • [ ] other

Current behavior

Check the repro on https://github.com/socketio/socket.io/issues/5135

New behavior

  • When the next message is sent, the system will realise it's missed the heartbeat, create a new connection, and then the message will be sent over the new connection
  • So now a message will never be sent over a stale connection, and we're not depending on the heartbeat timeout to fire correctly when a page comes back to life (which doesn't happen when it's throttled)

Other information (e.g. related issues)

The reason that the heartbeat logic is not immediately closing the old connection when the page is woken up is due to chrome pausing the setTimeout that's used in the heartbeat logic. This PR doesn't fix that. I Think that could be handled separately.

https://github.com/socketio/socket.io/issues/3507#issuecomment-977681297 is another fix related to throttled timers

Let me know if you think this makes sense and I'm happy to look into adding a test for it

tjenkinson avatar Jul 15 '24 13:07 tjenkinson

Looking at CI maybe this needs to be 2 separate PR's?

tjenkinson avatar Aug 22 '24 11:08 tjenkinson

Merged as https://github.com/socketio/socket.io/commit/8adcfbfde50679095ec2abe376650cf2b6814325.

I've made some minor edits and added a few tests. Could you please check if everything makes sense?

darrachequesne avatar Sep 18 '24 09:09 darrachequesne

Merged as 8adcfbf.

I've made some minor edits and added a few tests. Could you please check if everything makes sense?

Thanks! Just this one comment https://github.com/socketio/socket.io/commit/8adcfbfde50679095ec2abe376650cf2b6814325#r146871962

tjenkinson avatar Sep 18 '24 11:09 tjenkinson

Closing given merged in 8adcfbf 🎉

tjenkinson avatar Sep 24 '24 08:09 tjenkinson