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 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

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

@@ -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

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

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

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

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

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

properly handle multiple onData events
replace Object.keys(headers).forEach
remove nested inner functions
darrachequesne pushed a commit that referenced this pull request Feb 23, 2022
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.

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.

2 participants