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

test: [http] throw on arrays for Http2ServerResponse.end #33146

Closed
wants to merge 9 commits into from
Closed

test: [http] throw on arrays for Http2ServerResponse.end #33146

wants to merge 9 commits into from

Conversation

rexagod
Copy link
Member

@rexagod rexagod commented Apr 29, 2020

Refs: #29829

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 the http2 Issues or PRs related to the http2 subsystem. label Apr 29, 2020
@addaleax
Copy link
Member

@rexagod Can you explain this one? I’m already getting a type error thrown for the test here, just not the same one.

If this is about supporting the same types for the chunk argument, then we should actually fix this and allow Uint8Array input in HTTP instead. (We should do that anyway, to be clear.)

addaleax
addaleax reviewed Apr 30, 2020
View reviewed changes
lib/_http_outgoing.js Outdated Show resolved Hide resolved
@rexagod rexagod mentioned this pull request Apr 30, 2020
Closed
3 tasks
addaleax
addaleax reviewed May 2, 2020
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

HTTP/1 changes look good to me, but I think the HTTP/2 changes aren”t necessary because HTTP/2 is already doing the right thing here (but correct me if I’m wrong)

@rexagod
Copy link
Member Author

rexagod commented May 2, 2020

The main objective of this PR was to throw an error when an array is passed, which currently, is not thrown and no error event is emitted for. I'm not sure what you mean by HTTP2 is already doing the right thing here?

@addaleax
Copy link
Member

addaleax commented May 2, 2020

@rexagod But I’m already getting this error on master with your test:

_stream_writable.js:289
      throw new ERR_INVALID_ARG_TYPE(
      ^

TypeError [ERR_INVALID_ARG_TYPE]: The "chunk" argument must be of type string or an instance of Buffer or Uint8Array. Received an instance of Array
    at ServerHttp2Stream.Writable.write (_stream_writable.js:289:13)
    at Http2ServerResponse.write (internal/http2/compat.js:678:19)
    at Http2ServerResponse.end (internal/http2/compat.js:699:12)
[...]

i.e. writing Arrays is already forbidden either way.

@rexagod
Copy link
Member Author

rexagod commented May 3, 2020 •
edited
Loading

Done. It seems my local version was outdated.

@BridgeAR
Copy link
Member

BridgeAR commented May 3, 2020

Seems like this needs a test case?

addaleax
addaleax approved these changes May 7, 2020
View reviewed changes
lib/_http_outgoing.js Outdated Show resolved Hide resolved
@rexagod rexagod changed the title http2: add type checks for Http2ServerResponse.end test: throw on arrays for Http2ServerResponse.end May 10, 2020
@rexagod rexagod changed the title test: throw on arrays for Http2ServerResponse.end test: [http] throw on arrays for Http2ServerResponse.end May 10, 2020
@rexagod
Copy link
Member Author

rexagod commented May 10, 2020 •
edited
Loading

A PR that incorporated my changes was merged recently. So this PR is just for adding the test.

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/31519/

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 23, 2020
@BridgeAR
Copy link
Member

@rexagod this seem to fail on our CI. Please take another look.

@BridgeAR BridgeAR removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 23, 2020
@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 27, 2020
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 27, 2020 •
edited by BridgeAR
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/31587/ 💚

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request May 30, 2020
@rexagod @BridgeAR
http2: add type checks for Http2ServerResponse.end
de35c03 
Refs: nodejs#29829

PR-URL: nodejs#33146
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@BridgeAR
Copy link
Member

Landed in de35c03

@BridgeAR BridgeAR closed this May 30, 2020
@rexagod rexagod mentioned this pull request Jun 6, 2020
Open
32 tasks
codebytere pushed a commit that referenced this pull request Jun 18, 2020
@rexagod @codebytere
http2: add type checks for Http2ServerResponse.end
0b34c4f 
Refs: #29829

PR-URL: #33146
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@codebytere codebytere mentioned this pull request Jun 28, 2020
Merged
codebytere pushed a commit that referenced this pull request Jun 30, 2020
@rexagod @codebytere
http2: add type checks for Http2ServerResponse.end
a2c42b0 
Refs: #29829

PR-URL: #33146
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
codebytere pushed a commit that referenced this pull request Jul 8, 2020
@rexagod @codebytere
http2: add type checks for Http2ServerResponse.end
8def934 
Refs: #29829

PR-URL: #33146
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@codebytere codebytere mentioned this pull request Jul 13, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BridgeAR BridgeAR BridgeAR approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@rexagod @addaleax @BridgeAR @nodejs-github-bot @jasnell