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

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

Closed
wants to merge 4 commits into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Dec 14, 2019

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

@nodejs-github-bot nodejs-github-bot added the http2 Issues or PRs related to the http2 subsystem. label Dec 14, 2019
@ronag
Copy link
Member Author

ronag commented Dec 14, 2019

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

@ronag

This comment has been minimized.

@ronag
Copy link
Member Author

ronag commented Dec 14, 2019

Unsure about semver. Maybe major?

@ronag ronag changed the title http2: make HTTP2ServerResponse more streams compliant http2: make HTTP2ServerResponse.write more streams compliant Dec 14, 2019
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.

I’m good with semver-patch

@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

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

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.

lib/internal/http2/compat.js Outdated Show resolved Hide resolved
lib/internal/http2/compat.js Outdated Show resolved Hide resolved
lib/internal/http2/compat.js Outdated Show resolved Hide resolved
@ronag
Copy link
Member Author

ronag commented Dec 14, 2019

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

this.destroy(err);
return;
return false;
} else if (this[kState].closed) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the biggest issue was the throw.

@apapirovski apapirovski dismissed their stale review December 14, 2019 20:54

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

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

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

I believe this is author-ready?

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 25, 2019
@BridgeAR BridgeAR removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 25, 2019
@Trott
Copy link
Member

Trott commented Dec 31, 2019

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
@ronag
Copy link
Member Author

ronag commented Jan 1, 2020

rebased

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

nodejs-github-bot commented Jan 1, 2020

BridgeAR pushed a commit that referenced this pull request Jan 1, 2020
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 <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@BridgeAR
Copy link
Member

BridgeAR commented Jan 1, 2020

Landed in a1d307f 🎉

@BridgeAR BridgeAR closed this Jan 1, 2020
BridgeAR pushed a commit that referenced this pull request Jan 3, 2020
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 <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Jan 7, 2020
@targos
Copy link
Member

targos commented Jan 14, 2020

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

sxa pushed a commit to sxa/node that referenced this pull request Jan 21, 2020
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
sxa pushed a commit to sxa/node that referenced this pull request Jan 21, 2020
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
MylesBorins pushed a commit that referenced this pull request Jan 30, 2020
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 <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
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 <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@FurChan
Copy link

FurChan commented Dec 4, 2020

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.

``

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

9 participants