Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix AsyncWebSocketControl sending the same ping control packet more than once, breaking websocket sending altogether. #1390

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

2bam
Copy link

@2bam 2bam commented Mar 29, 2024

Problem

AsyncWebSocketControl packets were being sent more than once by AsyncWebSocketClient::_runQueue().

This blocks sending queued packets and eventually trigger ERROR: Too many messages queued serial message.

This PR adds a guard to avoid re-sending, similar to what AsyncWebSocketBasicMessage and AsyncWebSocketMultiMessage counterparts already do.


Cause

Couldn't track down or reason why it was happening, but I traced it via serial empirically.
The side effect when the same control packet is sent twice is described next.

Scenario

Let's say you send the same 2-byte control packet twice (by error). And also a 10-byte message once.
To clarify, there is only ONE control packet in the control queue, and only one message in the message queue.
Control packets get priority and are sent first, thus also ack'ed first.

AsyncWebSocketClient::_onAck callback will remove the first control packet from the control queue.
Then either

  • on the same _onAck (4-byte ack) call, due to len leftover to ack the second repeated packet...
  • or on the following _onAck (2-byte ack) call as the control queue is now empty and goes directly to the message...

...make the 10-byte message incorrectly ack'ed partially for 2-bytes from the second (wrong) control packet.

Then a following _onAck call, with the 10-bytes length for the message, will overflow the message as ack'ed for 12 bytes!
This breaks _runQueue() as mesg.betweenFrames() forever is false ( _ack == _acked turns into 10 == 12).

Consequences

AsyncWebSocketClient::_runQueue stops working.

  • Control queuing will never send controls as the there is a front message in the message queue which will be in perpetual NOT betweenFrames, inhibiting control send.
  • Message queuing knows it has already sent everything, and it's just waiting ack for the front message, blocking the rest from being sent.

The control queue can grow and grow, making the device run slower.
The message queue at some point starts issuing ERROR: Too many messages queued to the serial output.
Messages/controls are never sent again.

Follow up

I must also recommend adding an error flag or serial message if there is any len leftover on an _onAck as if it's ever caused by something else it will very likely lead to stalling like in this scenario.

@2bam 2bam changed the title Fix AsyncWebSocketControl repeated send Fix AsyncWebSocketControl sending the same control packet more than once. Mar 29, 2024
@2bam 2bam changed the title Fix AsyncWebSocketControl sending the same control packet more than once. Fix AsyncWebSocketControl sending the same ping control packet more than once, breaking websocket sending altogether. Mar 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant