Skip to content

http2: [compat api] add writable* properties - #33506

Closed
rexagod wants to merge 3 commits into
nodejs:masterfrom
rexagod:i-29829-09-10-11
Closed

http2: [compat api] add writable* properties#33506
rexagod wants to merge 3 commits into
nodejs:masterfrom
rexagod:i-29829-09-10-11

Conversation

@rexagod

@rexagodrexagod commented May 22, 2020

Copy link
Copy Markdown
Member

added writableHighWaterMark, writableLength, and writableFinished
properties with test.

Refs: #29829 (9th, 10th, and 11th sub-issues)

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

added writableHighWaterMark, writableLength, and writableFinished
properties with test.
Refs: nodejs#29829
@nodejs-github-botnodejs-github-bot added the http2 Issues or PRs related to the http2 subsystem. label May 22, 2020
@rexagodrexagod closed this May 22, 2020
}

get writableFinished() {
return this[kStream].writableFinished;

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.

I suspect this will need a special case and test for state.headRequest.

@ronagronagMay 24, 2020

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.

Though I think headRequest is slightly broken or maybe I don't understand it. There is special handling for it in Http2ServerResponse.end() but not in Http2ServerResponse.write()? Not sure who is a good ping there. @jasnell?

@rexagodrexagodMay 24, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

EDIT: please create an issue if landing this PR without resolving this.

Issue: #33543

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

LGTM, added a comment for state.headRequest but I would be fine with sorting that out in a follow up PR if it is difficult to resolve.

EDIT: please create an issue if landing this PR without resolving this.

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

lgtm

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@BridgeARBridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 24, 2020
Comment threadtest/parallel/test-http2-res-writable-properties.js Outdated
Co-authored-by: Luigi Pinca <luigipinca@gmail.com>
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request May 30, 2020
added writableHighWaterMark, writableLength, and writableFinished
properties with test.
Refs: nodejs#29829
PR-URL: nodejs#33506
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@BridgeAR

Copy link
Copy Markdown
Member

Landed in 99abaf9

codebytere pushed a commit that referenced this pull request Jun 18, 2020
added writableHighWaterMark, writableLength, and writableFinished
properties with test.
Refs: #29829
PR-URL: #33506
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@codebyterecodebytere mentioned this pull request Jun 28, 2020
codebytere pushed a commit that referenced this pull request Jun 30, 2020
added writableHighWaterMark, writableLength, and writableFinished
properties with test.
Refs: #29829
PR-URL: #33506
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
codebytere pushed a commit that referenced this pull request Jul 8, 2020
added writableHighWaterMark, writableLength, and writableFinished
properties with test.
Refs: #29829
PR-URL: #33506
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@codebyterecodebytere mentioned this pull request Jul 13, 2020
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.http2Issues or PRs related to the http2 subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@rexagod@nodejs-github-bot@BridgeAR@mcollina@lpinca@ronag