Skip to content

revert: "websocket: pre-calculated length" - #3290

Merged
KhafraDev merged 2 commits into
nodejs:mainfrom
KhafraDev:revert-fragment-length
May 21, 2024
Merged

revert: "websocket: pre-calculated length"#3290
KhafraDev merged 2 commits into
nodejs:mainfrom
KhafraDev:revert-fragment-length

Conversation

@KhafraDev

@KhafraDev KhafraDev commented May 21, 2024

Copy link
Copy Markdown
Member

This reverts 13523fd which broke permessage-deflate support.

See #3289 for why the tests did not catch this.

This comment was marked as outdated.

ghost left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a test to avoid further regressions?

ghost commented May 21, 2024

Copy link
Copy Markdown
Member Author

The tests catch it already, it's just that the Autobahn workflow doesn't quite work as expected and it uses a previous commit rather than the current one. Once the issue linked is resolved it'll be fixed.

ghost commented May 21, 2024

Copy link
Copy Markdown
Member Author

The tests in #3283 are failing because of this, or you can see the first comment that the bot made before the edit. There is a lot broken right now I'd appreciate if we could land this in the meantime.

ghost commented May 21, 2024

Copy link
Copy Markdown

@mcollina
Please remove your blocking review.

@ghost ghost mentioned this pull request May 21, 2024
Khafra added 2 commits May 21, 2024 16:02
This reverts 13523fd which broke
permessage-deflate support.

See nodejs#3289 for why the tests did
not catch this.
@ghost
ghost force-pushed the revert-fragment-length branch from 7c4908c to 93c653e Compare May 21, 2024 20:02

ghost commented May 21, 2024

Copy link
Copy Markdown

@KhafraDev
Is it normal that the autobahn workflow is so slow?

ghost commented May 21, 2024

Copy link
Copy Markdown
Member Author

Yes, you have to understand that these tests send GB of frames over that need to parsed and mirrored to the server. The longer the tests, the more likely the tests are to succeed because the server will instantly fail/close the connection if it receives something unexpected.

ghost commented May 21, 2024

Copy link
Copy Markdown

Ah, ok. ;)

@ghost
ghost dismissed mcollina’s stale review May 21, 2024 20:14

the autobahn workflow runs tests that detected this regression originally

ghost commented May 21, 2024

Copy link
Copy Markdown
Member Author

cc @tsctx I'm pretty positive that the issue came with the custom Buffer.concat.

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.

3 participants