http2: make http2/compat.write more http/1 compliant by ronag · Pull Request #30964 · nodejs/node · GitHub

HTTP2ServerResponse.write would behave differently than both http1 and streams. This PR makes it more compliant with stream.Writable behaviour.

In particular, prior to this PR, write would throw err instead of calling destroy(err)

Refs: #29529

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Would be nice to eventually consolidate stream, http1, http/2 compat and http/2 in terms of the streams API & behaviours.

Unsure about semver. Maybe major?

I’m good with semver-patch

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

I don't think ERR_STREAM_DESTROYED belongs here. Also left some nits but that's the primary objection for me.

Would also like James to review ideally.

@apapirovski: I think you prefer to align with http1 instead of streams. I've updated the PR accordingly.

My other outstanding question regarding closed stands. Might be good to get @jasnell @addaleax to weigh in too. Seems like this existing error is not in the spirit of compatibility which could cause people issues...

(Could also be left for a separate PR.)

(It was mentioned in the linked issue, hence me bringing it up.)

I'll leave this as is until there is more input.

I think the biggest issue was the throw.

change request was addressed, will approve after outstanding discussion is resolved

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

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

I believe this is author-ready?

Needs a rebase.

HTTP2ServerResponse.write would behave differently than both http1 and streams. This PR makes it more compliant with stream.Writable behaviour. Refs: nodejs#29529

rebased

CI: https://ci.nodejs.org/job/node-test-pull-request/28069/ ✔️

HTTP2ServerResponse.write would behave differently than both http1 and streams. This PR makes it more compliant with stream.Writable behaviour. PR-URL: #30964 Refs: #29529 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com>

Landed in a1d307f 🎉

HTTP2ServerResponse.write would behave differently than both http1 and streams. This PR makes it more compliant with stream.Writable behaviour. PR-URL: #30964 Refs: #29529 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com>

This needs a backport or other previous PRs to be backported in order to land on v12.x-staging.

HTTP2ServerResponse.write would behave differently than both http1 and streams. This PR makes it more compliant with stream.Writable behaviour. PR-URL: nodejs#30964 Refs: nodejs#29529

HTTP2ServerResponse.write would behave differently than both http1 and streams. This PR makes it more compliant with stream.Writable behaviour. PR-URL: nodejs#30964 Refs: nodejs#29529 Backport-PR-URL: nodejs#31444

HTTP2ServerResponse.write would behave differently than both http1 and streams. This PR makes it more compliant with stream.Writable behaviour. Backport-PR-URL: #31444 PR-URL: #30964 Refs: #29529 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com>

This needs a backport or other previous PRs to be backported in order to land on v12.x-staging.

This issue is still present on 12.20.0. I think the backported-to-v12.x tag might be misapplied here, or the fix was not sufficient.

``

nodejs-github-bot added the http2 Issues or PRs related to the http2 subsystem. label Dec 14, 2019

ronag mentioned this pull request Dec 14, 2019

HTTP2 compatibility layer issues with invalid streams #29529

Closed

This comment has been minimized.

Sign in to view

ronag force-pushed the http2-compat-writable branch from 69f9a1d to dac542b Compare December 14, 2019 18:18

ronag changed the title http2: make HTTP2ServerResponse more streams compliant http2: make HTTP2ServerResponse.write more streams compliant Dec 14, 2019

addaleax approved these changes Dec 14, 2019

View reviewed changes

apapirovski previously requested changes Dec 14, 2019

View reviewed changes

Comment thread lib/internal/http2/compat.js Outdated

Comment thread lib/internal/http2/compat.js Outdated

Comment thread lib/internal/http2/compat.js Outdated

ronag force-pushed the http2-compat-writable branch from d1a706e to 1ef5224 Compare December 14, 2019 19:44

apapirovski reviewed Dec 14, 2019

View reviewed changes

Comment thread lib/internal/http2/compat.js Outdated

ronag force-pushed the http2-compat-writable branch from c8c1f00 to 19c30e7 Compare December 14, 2019 19:56

ronag changed the title http2: make HTTP2ServerResponse.write more streams compliant http2: make http2/compat.write more http/1 compliant Dec 15, 2019

Trott approved these changes Dec 18, 2019

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Dec 25, 2019

BridgeAR requested a review from jasnell December 25, 2019 10:38

BridgeAR removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Dec 25, 2019

ronag added 4 commits January 1, 2020 12:18

http2: make HTTP2ServerResponse more streams compliant

e09aa04

HTTP2ServerResponse.write would behave differently than both http1 and streams. This PR makes it more compliant with stream.Writable behaviour. Refs: nodejs#29529

fixup: align with http1 instead of streams

4684a1e

fixup: cleaner version

2f0d4b1

fixup: removed destroyed test

9da63be

ronag force-pushed the http2-compat-writable branch from 493c71a to 9da63be Compare January 1, 2020 11:20

BridgeAR added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Jan 1, 2020

BridgeAR closed this Jan 1, 2020

BridgeAR mentioned this pull request Jan 7, 2020

v13.6.0 proposal #31238

Merged

targos added the backport-requested-v12.x label Jan 14, 2020

sxa mentioned this pull request Jan 21, 2020

[v12.x backport] http2: make HTTP2ServerResponse more streams compliant #31444

Closed

4 tasks

sogaani mentioned this pull request Jan 28, 2020

The differences between http2 compatibility API and http[s] #29829

Closed

32 tasks

MylesBorins added backported-to-v12.x and removed backport-requested-v12.x labels Jan 30, 2020

http2: make http2/compat.write more http/1 compliant by ronag · Pull Request #30964 · nodejs/node · GitHub

Uh oh!

Uh oh!

http2: make http2/compat.write more http/1 compliant - #30964

http2: make http2/compat.write more http/1 compliant#30964ronag wants to merge 4 commits intonodejs:masternodejs/node:masterfrom nxtedition:http2-compat-writablenxtedition/node:http2-compat-writableCopy head branch name to clipboard

Conversation

ronag commented Dec 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

ronag commented Dec 14, 2019

Uh oh!

This comment has been minimized.

ronag commented Dec 14, 2019

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Dec 14, 2019

Uh oh!

apapirovski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ronag commented Dec 14, 2019

Uh oh!

apapirovski Dec 14, 2019

Choose a reason for hiding this comment

Uh oh!

apapirovski Dec 14, 2019

Choose a reason for hiding this comment

Uh oh!

ronag Dec 14, 2019

Choose a reason for hiding this comment

Uh oh!

ronag Dec 14, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nodejs-github-bot commented Dec 14, 2019

Uh oh!

nodejs-github-bot commented Dec 14, 2019

Uh oh!

BridgeAR commented Dec 24, 2019

Uh oh!

Trott commented Dec 31, 2019

Uh oh!

ronag commented Jan 1, 2020

Uh oh!

nodejs-github-bot commented Jan 1, 2020 • edited by BridgeAR Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BridgeAR commented Jan 1, 2020

Uh oh!

targos commented Jan 14, 2020

Uh oh!

FurChan commented Dec 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

http2: make http2/compat.write more http/1 compliant#30964
ronag wants to merge 4 commits into
nodejs:masterfrom
nxtedition:http2-compat-writable

ronag commented Dec 14, 2019 •
edited
Loading

nodejs-github-bot commented Jan 1, 2020 •
edited by BridgeAR
Loading

FurChan commented Dec 4, 2020 •
edited
Loading