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.

Sign up for GitHub

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

Jump to bottom

fix uws polling #642

Closed
wants to merge 2 commits into from
Closed

fix uws polling #642

wants to merge 2 commits into from

Conversation

e3dio
Copy link
Contributor

@e3dio e3dio commented Feb 18, 2022 •
edited
Loading

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

Polling error, see socketio/socket.io#4281

New behaviour

Prevent error, also improves performance: reduces copies from 2 to 0 or 1, replaces Object.keys(headers).forEach, removes nested inner functions

@e3dio
Copy link
Contributor Author

e3dio commented Feb 18, 2022

I don't use Socket.io and am not familiar with building and testing it so this is not tested at all, should work but should be reviewed and tested

@e3dio e3dio mentioned this pull request Feb 18, 2022
Closed
darrachequesne
darrachequesne reviewed Feb 20, 2022
View reviewed changes
lib/transports-uws/polling.ts
@@ -122,6 +122,18 @@ export class Polling extends Transport {
return;
}

const contentLengthHeader = Number(req.headers["content-length"]);
Copy link
Member

Choose a reason for hiding this comment

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

If the content is chunked (Transfer-Encoding: chunked), won't the Content-Length header be undefined there?

Copy link
Contributor Author

@e3dio e3dio Feb 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

transfer-encoding: chunked POST's from the client are not supported by uWS, see uNetworking/uWebSockets.js#669 it is only supported for responses. The Socket.io client needs to not use it for this case

Copy link
Member

@darrachequesne darrachequesne Feb 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

OK, thanks for the link. But in that case, what happened in the issue? Does it throw on the first chunk?

Copy link
Contributor Author

@e3dio e3dio Feb 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I see there is specific http code 411 for Length Required, I will update that https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/411 then the client knows to use length

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I tested this with new Node v17.5 fetch using { body: createReadStream() } to send transfer-encoding: chunked request, uWS.js successfully responds with status 411 Length Required when request is missing content-length header

@e3dio
Copy link
Contributor Author

e3dio commented Feb 22, 2022 •
edited
Loading

I changed existing Object.keys(headers).forEach to for (let key in headers) and removed the nested inner function cleanup, from my test on v8 declaring inner functions in a hot path does not hurt unless you nest them together like cleanup and onClose was doing

@e3dio
Copy link
Contributor Author

e3dio commented Feb 22, 2022 •
edited
Loading

I saw this comment from @darrachequesne on recent commit:

On Safari, a POST request would result in two chunks (the first being empty), and threw the following error:

TypeError: Cannot perform %TypedArray%.prototype.set on a detached ArrayBuffer

Which is a bit weird, because it seems µWebSockets.js does not support chunked content: uNetworking/uWebSockets.js#669

A normal POST does not use transfer-encoding: chunked, chunked is for when you don't know size of content you are sending, like streaming dynamic content, and is not related to this issue. This PR fixes any POST above a certain size that requires more than 1 onData event, as data gets split into chunks to send across network. transfer-encoding: chunked is a different protocol for sending data not related to this issue, you are mixing things up, you don't need to be accepting that type of stream for this endpoint

d970d66#comments

@e3dio
fix uws polling
fb11d83 
properly handle multiple onData events
@e3dio
perf update
9ccbc34 
replace Object.keys(headers).forEach
remove nested inner functions
darrachequesne pushed a commit that referenced this pull request Feb 23, 2022
@e3dio @darrachequesne
fix(uws): properly handle chunked content (#642)
3367440 
With the engine based on µWebSockets.js (introduced in version 6.1.0),
a huge request body split in multiple chunks would throw the following
error:

> node:buffer:254
>   TypedArrayPrototypeSet(target, source, targetStart);
>   ^
>
> TypeError: Cannot perform %TypedArray%.prototype.set on a detached ArrayBuffer
>     at Buffer.set (<anonymous>)
>     at _copyActual (node:buffer:254:3)
> node:buffer:254
>   TypedArrayPrototypeSet(target, source, targetStart);
>   ^
>
> TypeError: Cannot perform %TypedArray%.prototype.set on a detached ArrayBuffer
>     at Buffer.set (<anonymous>)
>     at _copyActual (node:buffer:254:3)
>     at Function.concat (node:buffer:562:12)
>     at onEnd (.../node_modules/engine.io/build/transports-uws/polling.js:126:32)
>     at .../node_modules/engine.io/build/transports-uws/polling.js:143:17

Note: µWebSockets.js does not currently support chunked transfer
encoding.
@darrachequesne
Copy link
Member

Merged as 3367440. Thanks.

@moul-bot moul-bot mentioned this pull request Oct 2, 2022
Closed
@ekmixon ekmixon mentioned this pull request Nov 21, 2022
Open
@ekmixon ekmixon mentioned this pull request Oct 13, 2023
Open
@ekmixon ekmixon mentioned this pull request Dec 6, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@darrachequesne darrachequesne darrachequesne left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@e3dio @darrachequesne