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

http: don't throw on Uint8Arrays for http.ServerResponse#write #33155

Closed
wants to merge 2 commits into from

Conversation

rexagod
Copy link
Member

@rexagod rexagod commented Apr 30, 2020

Same as #33146 but for write

Refs: #29829
Fixes: #33379

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added http Issues or PRs related to the http subsystem. http2 Issues or PRs related to the http2 subsystem. labels Apr 30, 2020
@rexagod rexagod force-pushed the i-29829-15 branch 3 times, most recently from 4cf2098 to 7ce222f Compare April 30, 2020 04:56
Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

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

I would prefer if you tried to make it as similar as possible to what's in Writable.

Also, please try to avoid unrelated whitespace changes.

@rexagod rexagod changed the title http2: add checks for Http2Server.Response.write http2: add type checks for Http2Server.Response.write May 1, 2020
@addaleax
Copy link
Member

addaleax commented May 2, 2020

Yeah, I think this should also change the HTTP/1 code and leave HTTP/2 alone because that’s already doing the right thing, like #33146 👍

@rexagod
Copy link
Member Author

rexagod commented May 2, 2020

@addaleax Sorry but I'm confused as to what you are referring to the 'right thing'. If I'm understanding this right, do you want me to remove these type checks for write (in http2), and what should I change in http/1 (since it already has type checks)?

@addaleax
Copy link
Member

addaleax commented May 2, 2020

@rexagod Fit the HTTP/1 code so that it also accepts any type of Uint8Array (i.e. replace chunk instanceof Buffer with isUint8Array and fix up the error message, just like #33146), and don’t modify the HTTP/2 code because that passes the chunk along to Writable.prototype.write, which already performs correct typechecking (= chunk is string or Uint8Array).

@rexagod rexagod force-pushed the i-29829-15 branch 2 times, most recently from 56a56f0 to b253d17 Compare May 3, 2020 06:08
@rexagod
Copy link
Member Author

rexagod commented May 3, 2020

Thank you so much for clearing this up for me, @addaleax! Sometimes I miss to see the obvious and it's really frustrating! 😅

lib/_http_outgoing.js Outdated Show resolved Hide resolved
test/parallel/test-http-outgoing-write-types.js Outdated Show resolved Hide resolved
@BridgeAR
Copy link
Member

BridgeAR commented May 3, 2020

@ronag PTAL. I think your comment got resolved.

@rexagod rexagod force-pushed the i-29829-15 branch 2 times, most recently from 83686ad to fde814e Compare May 10, 2020 08:38
lib/_http_outgoing.js Outdated Show resolved Hide resolved
Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

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

This is changing http1 which already has type checks. Not sure if the Uint8Array check works so that might be worth fixing.

@rexagod
Copy link
Member Author

rexagod commented May 10, 2020

@ronag Should I remove all changes from outgoing and just commit the test?

@ronag
Copy link
Member

ronag commented May 10, 2020

@ronag Should I remove all changes from outgoing and just commit the test?

Could you also add a test for the uint8array case? Also please fix the commit msg and PR description.

@rexagod rexagod force-pushed the i-29829-15 branch 2 times, most recently from 3872384 to 33484b2 Compare May 10, 2020 16:57
dont throw errors on Uint8Arrays and added test for all
valid types

Co-authored-by: Ruben Bridgewater <ruben@bridgewater.de>

fixup: remove dup

fixup: remove unused dec

fixup: add Uint8Array check in test

fixup: don't throw invalid arg error on `Uint8Array`s
@ronag ronag changed the title http2: add type checks for Http2Server.Response.write http: don't throw on Uint8Arrays for http.ServerResponse#write May 10, 2020
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@targos
Copy link
Member

targos commented May 16, 2020

Landed in 2b50cd7

@targos targos closed this May 16, 2020
targos pushed a commit that referenced this pull request May 16, 2020
Don't throw errors on Uint8Arrays and added test for all
valid types.

PR-URL: #33155
Fixes: #33379
Refs: #29829
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@codebytere
Copy link
Member

@rexagod should this go back to v14.x? It changes something introduced in #31818, which is semver-major.

@addaleax
Copy link
Member

@codebytere Yes, it just needs to be applied in a different part of the function that #31818 had moved around (but not really introduced).

@rexagod Would you be willing to backport this yourself? The guide for how to do that is in https://github.com/nodejs/node/blob/master/doc/guides/backporting-to-release-lines.md. If you’d prefer, you can also let us know that somebody else should pick that up.

rexagod added a commit to rexagod/node that referenced this pull request May 20, 2020
Don't throw errors on Uint8Arrays and added test for all
valid types.

Backport-PR-URL: nodejs#33488
PR-URL: nodejs#33155
Fixes: nodejs#33379
Refs: nodejs#29829
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
rexagod added a commit to rexagod/node that referenced this pull request May 20, 2020
Don't throw errors on Uint8Arrays and added test for all
valid types.

Backport-PR-URL: nodejs#33490
PR-URL: nodejs#33155
Fixes: nodejs#33379
Refs: nodejs#29829
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@rexagod
Copy link
Member Author

rexagod commented May 20, 2020

ping @addaleax

codebytere pushed a commit that referenced this pull request Jun 27, 2020
Don't throw errors on Uint8Arrays and added test for all
valid types.

Backport-PR-URL: #33490
PR-URL: #33155
Fixes: #33379
Refs: #29829
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@codebytere codebytere mentioned this pull request Jun 28, 2020
codebytere pushed a commit that referenced this pull request Jun 30, 2020
Don't throw errors on Uint8Arrays and added test for all
valid types.

Backport-PR-URL: #33490
PR-URL: #33155
Fixes: #33379
Refs: #29829
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Aug 18, 2020
Don't throw errors on Uint8Arrays and added test for all
valid types.

Backport-PR-URL: #33488
PR-URL: #33155
Fixes: #33379
Refs: #29829
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jimmywarting
Copy link

jimmywarting commented Sep 15, 2020

The title says http.ServerResponse#write
but how about http.ClientRequest? can you write uint8 to request also? currently don't work in node v12

@codebytere codebytere mentioned this pull request Sep 28, 2020
@targos targos added backported-to-v14.x and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backport-requested-v14.x labels Apr 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem. http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ServerResponse.write throws an exception when passing a Uint8array.
10 participants