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 websocket and webtransport multipart callbacks (#698) #699

Merged
merged 3 commits into from
Jun 13, 2024

Conversation

jonathanperret
Copy link
Contributor

The kind of change this PR does introduce

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

Current behaviour

When using the websocket transport, some send callbacks are not called. See #698, and the tests added in the first commit of this PR. The webtransport transport was also found to have the same bug when testing.

New behaviour

All send callbacks are now called.

Also, the type for the callback argument to send() was enriched to reflect the fact that the transport is passed (as it always has been) as its first argument. This does change the types visible publicly, but TypeScript type compatibility rules should make this a non-breaking change (i.e. it's still OK to pass a callback expecting no arguments).

Other information

The bug was caused by the websocket transport (and webtransport as well) having its supportsFraming property set to true, despite having been changed in #618 to emit a single drain event for each batch of messages written to the transport like the polling transport always did. Note that although #618 is partially reverted in a65a047, the new drain event behavior is preserved as called out in that commit's message.

The supportsFraming attribute was introduced in #130 (amended by #132) as a way to distinguish transports that emit one drain per message from those that emit one drain per batch. Since the delivery of send callbacks depends on matching drain events with transport.send calls, that distinction is vital to correct behavior.

However, now that all transports have converged to "one drain per batch" behavior, this supportsFraming property can be retired (and the code for calling callbacks simplified), and this is the refactoring that the last commit of this PR proposes. This is technically a breaking change since supportsFraming was exposed in the TypeScript types of engine.io, but as far as I can tell it was never documented.

@darrachequesne darrachequesne merged commit fc21c4a into socketio:main Jun 13, 2024
2 checks passed
@darrachequesne
Copy link
Member

@jonathanperret thanks a lot for the detailed analysis ❤️ I could indeed reproduce the issue.

I was not able to reproduce the claims from #618, so I guess we can safely remove the supportsFraming attribute.

@jonathanperret jonathanperret deleted the fix-multipart-callbacks branch June 14, 2024 10:19
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.

None yet

2 participants