-
Notifications
You must be signed in to change notification settings - Fork 28.9k
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: fix no response event on continue request #41739
http2: fix no response event on continue request #41739
Conversation
Review requested:
|
Hey @ofirbarak thanks for tackling this, I am not sure the fix is quite correct - I see there are some relevant test failures? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Welcome, @ofirbarak and thanks for this pull request as well! As with the other one, please change the start of the first commit message from lib:
to http2:
. Thanks!
If another collaborator wants to land this before that change, feel free to dismiss this review but please change the commit message while landing. Thanks!
When sending a continue request, server response with null, it does not fires the response event type Fixes: nodejs#38258
7bc8b08
to
91db9b4
Compare
@jasnell could you take a look before we land? |
Commit Queue failed- Loading data for nodejs/node/pull/41739 ✔ Done loading data for nodejs/node/pull/41739 ----------------------------------- PR info ------------------------------------ Title http2: fix no response event on continue request (#41739) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch ofirbarak:http2-null-response -> nodejs:master Labels http2 Commits 4 - http2: fix no response event on continue request - fix liniting issues - correct the fix - simplify condition Committers 1 - ofir PR-URL: https://github.com/nodejs/node/pull/41739 Fixes: https://github.com/nodejs/node/issues/38258 Refs: https://github.com/nodejs/node/pull/38561 Reviewed-By: Benjamin Gruenbaum Reviewed-By: Matteo Collina Reviewed-By: James M Snell ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/41739 Fixes: https://github.com/nodejs/node/issues/38258 Refs: https://github.com/nodejs/node/pull/38561 Reviewed-By: Benjamin Gruenbaum Reviewed-By: Matteo Collina Reviewed-By: James M Snell -------------------------------------------------------------------------------- ℹ This PR was created on Sat, 29 Jan 2022 00:22:36 GMT ✔ Approvals: 3 ✔ - Benjamin Gruenbaum (@benjamingr): https://github.com/nodejs/node/pull/41739#pullrequestreview-866996187 ✔ - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/41739#pullrequestreview-867008165 ✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/41739#pullrequestreview-871911901 ⚠ GitHub cannot link the author of 'http2: fix no response event on continue request' to their GitHub account. ⚠ Please suggest them to take a look at https://github.com/nodejs/node/blob/99b1ada/doc/guides/contributing/pull-requests.md#step-1-fork ⚠ GitHub cannot link the author of 'fix liniting issues' to their GitHub account. ⚠ Please suggest them to take a look at https://github.com/nodejs/node/blob/99b1ada/doc/guides/contributing/pull-requests.md#step-1-fork ⚠ GitHub cannot link the author of 'correct the fix' to their GitHub account. ⚠ Please suggest them to take a look at https://github.com/nodejs/node/blob/99b1ada/doc/guides/contributing/pull-requests.md#step-1-fork ⚠ GitHub cannot link the author of 'simplify condition' to their GitHub account. ⚠ Please suggest them to take a look at https://github.com/nodejs/node/blob/99b1ada/doc/guides/contributing/pull-requests.md#step-1-fork ✔ Last GitHub CI successful ℹ Last Full PR CI on 2022-02-03T09:48:20Z: https://ci.nodejs.org/job/node-test-pull-request/42332/ - Querying data for job/node-test-pull-request/42332/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/master up to date... From https://github.com/nodejs/node * branch master -> FETCH_HEAD ✔ origin/master is now up-to-date - Downloading patch for 41739 From https://github.com/nodejs/node * branch refs/pull/41739/merge -> FETCH_HEAD ✔ Fetched commits as 28989b4103d7..91db9b4d8b22 -------------------------------------------------------------------------------- [master 69c13726d0] http2: fix no response event on continue request Author: ofir Date: Sat Jan 29 02:14:00 2022 +0200 2 files changed, 98 insertions(+), 55 deletions(-) rewrite test/parallel/test-http2-compat-expect-continue.js (85%) [master a05210f0c0] fix liniting issues Author: ofir Date: Sat Jan 29 03:11:06 2022 +0200 2 files changed, 2 insertions(+), 2 deletions(-) [master 9168f2a135] correct the fix Author: ofir Date: Sat Jan 29 13:39:06 2022 +0200 2 files changed, 4 insertions(+), 4 deletions(-) [master 02e3fa3a4e] simplify condition Author: ofir Date: Sun Jan 30 15:22:04 2022 +0200 1 file changed, 1 insertion(+), 4 deletions(-) ✔ Patches applied There are 4 commits in the PR. Attempting autorebase. Rebasing (2/8)https://github.com/nodejs/node/actions/runs/1789949374 |
Landed in be6844d |
Congrats on your first contribution to Node core @ofirbarak 🎉 |
Thank you! |
When sending a continue request, server response with null, it does not fires the response event type Fixes: nodejs#38258 PR-URL: nodejs#41739 Refs: nodejs#38561 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
When sending a continue request, server response with null, it does not fires the response event type Fixes: #38258 PR-URL: #41739 Refs: #38561 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
When sending a continue request, server response with null, it does not fires the response event type Fixes: #38258 PR-URL: #41739 Refs: #38561 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
When sending a continue request, server response with null, it does not fires the response event type Fixes: #38258 PR-URL: #41739 Refs: #38561 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
When sending a continue request, server response with null, it does not fires the response event type Fixes: nodejs#38258 PR-URL: nodejs#41739 Refs: nodejs#38561 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
When sending a continue request, server response with null, it does not fires the response event type Fixes: nodejs#38258 PR-URL: nodejs#41739 Refs: nodejs#38561 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
When sending a continue request, server response with null, it does not fires the response event type Fixes: #38258 PR-URL: #41739 Refs: #38561 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
When sending a continue request, server response with null, it does not fires the response event type Fixes: #38258 PR-URL: #41739 Refs: #38561 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
When sending a continue request, server response with null, but it does not fires the response event type
Fixes: #38258
Ref: #38561