http2: add support for AbortSignal to http2Session.request#36070
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot commented Nov 10, 2020
Review requested:
|
Uh oh!
There was an error while loading. Please reload this page.
benjamingr commented Nov 10, 2020
@aduh95 this is HTTP2 and the one I opened is HTTP :) I spoke with Dor and suggested this as a contribution opportunity |
benjamingr commented Nov 10, 2020
Ping @jasnell on http2 + AbortController :] |
Uh oh!
There was an error while loading. Please reload this page.
github-actionsbot removed the request-ci nodejs-github-bot commented Nov 10, 2020
lib/internal/http2/core.js Outdated
There was a problem hiding this comment.
I would prefer this to be called with an error.
There was a problem hiding this comment.
I've been following the discussion on #36048 on the same topic. I don't know enough about the specs and surrounding discussion to have an opinion either way, but over there it looks like it's not being called with an error either (at least for now).
Whatever the decision is it should probably be consistent in both http and http2 😄, whatever you guys decide I'm game.
cc @benjamingr
There was a problem hiding this comment.
This already terminates with an error though and there is a test for that? Destroying the stream early (even without an explicit error on the http2 request) emits an error (right?)
There was a problem hiding this comment.
Yes, but I think destroying with an explicit aborted DOMException like we do elsewhere is good for consistency here.
There was a problem hiding this comment.
@jasnell but wouldn't the request get aborted with an ECONNRESET (like in the test) anyway?
There was a problem hiding this comment.
ECONNREFUSED if it's aborted on the same tick, ECONNRESET otherwise. (Also, think that should be enforced by the test?)
There was a problem hiding this comment.
Test appreciated, I think the errors a refine no?
github-actionsbot removed the request-ci nodejs-github-bot commented Nov 10, 2020
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
MadaraUchihaforce-pushed the add-abortsignal-http2-request branch from 3875bfb to dc97603Compare
benjamingr commented Nov 11, 2020
See additional discussion here: #36048 (comment) |
aduh95 commented Nov 17, 2020
This needs a rebase. |
MadaraUchihaforce-pushed the add-abortsignal-http2-request branch from dc97603 to db9ad63Compare- Remove redundant socket check - Add assertion that AbortSignal event listener gets added then removed.
- Builds on nodejs#36048 and nodejs#36084 - Modify test to verify this fact
- Calling abort no longer behaves like .destroy() - Fix linting errors
- Also add test to that effect
MadaraUchihaforce-pushed the add-abortsignal-http2-request branch from fe2b00e to 9aca7c4Comparegithub-actionsbot removed the request-ci nodejs-github-bot commented Nov 21, 2020
benjamingr added the commit-queue github-actionsbot added commit-queue-failed 
Commit Queue failed- Loading data for nodejs/node/pull/36070 ✔ Done loading data for nodejs/node/pull/36070 ----------------------------------- PR info ------------------------------------ Title http2: add support for AbortSignal to http2Session.request (#36070) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch MadaraUchiha:add-abortsignal-http2-request -> nodejs:master Labels http2 Commits 9 - http2: add support for AbortSignal to http2Session.request - http2: Document AbortSignalSupport - http2: Fix lint errors - http2: Remove redundant empty line in docs - http2: Use existing signal constant - http2: Goodify test - http2: Abort calls destroy with an AbortError - http2: Update documentation - http2: Abort immediately if passed an already aborted signal Committers 1 - Madara Uchiha PR-URL: https://github.com/nodejs/node/pull/36070 Reviewed-By: Matteo Collina Reviewed-By: Benjamin Gruenbaum ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/36070 Reviewed-By: Matteo Collina Reviewed-By: Benjamin Gruenbaum -------------------------------------------------------------------------------- ✔ Last GitHub Actions successful ℹ Last Full PR CI on 2020-11-21T10:44:25Z: https://ci.nodejs.org/job/node-test-pull-request/34501/ - Querying data for job/node-test-pull-request/34501/ ✔ Build data downloaded ✔ Last Jenkins CI successful ℹ This PR was created on Tue, 10 Nov 2020 17:09:52 GMT ✔ Approvals: 2 ✔ - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/36070#pullrequestreview-535274077 ✔ - Benjamin Gruenbaum (@benjamingr): https://github.com/nodejs/node/pull/36070#pullrequestreview-535962363 -------------------------------------------------------------------------------- ✔ 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 36070 From https://github.com/nodejs/node * branch refs/pull/36070/merge -> FETCH_HEAD ✔ Fetched commits as 03fd3634721e..9aca7c4e0f76 -------------------------------------------------------------------------------- [master 9f4c660e32] http2: add support for AbortSignal to http2Session.request Author: Madara Uchiha Date: Tue Nov 10 19:06:41 2020 +0200 2 files changed, 40 insertions(+) [master 5ee4ab7351] http2: Document AbortSignalSupport Author: Madara Uchiha Date: Tue Nov 10 19:14:04 2020 +0200 1 file changed, 10 insertions(+) [master 2be6c72532] http2: Fix lint errors Author: Madara Uchiha Date: Tue Nov 10 19:17:55 2020 +0200 2 files changed, 2 insertions(+), 2 deletions(-) [master 81dfc6af59] http2: Remove redundant empty line in docs Author: Madara Uchiha Date: Tue Nov 10 20:09:40 2020 +0200 1 file changed, 1 deletion(-) [master db67139056] http2: Use existing signal constant Author: Madara Uchiha Date: Tue Nov 10 21:41:02 2020 +0200 1 file changed, 1 insertion(+), 1 deletion(-) [master 244e6f6448] http2: Goodify test Author: Madara Uchiha Date: Wed Nov 11 00:22:01 2020 +0200 2 files changed, 11 insertions(+), 7 deletions(-) [master 158e3daeee] http2: Abort calls destroy with an AbortError Author: Madara Uchiha Date: Thu Nov 19 19:29:00 2020 +0200 2 files changed, 16 insertions(+), 9 deletions(-) [master 9c33a11777] http2: Update documentation Author: Madara Uchiha Date: Thu Nov 19 19:43:31 2020 +0200 2 files changed, 7 insertions(+), 7 deletions(-) [master f980a8d85b] http2: Abort immediately if passed an already aborted signal Author: Madara Uchiha Date: Thu Nov 19 21:00:33 2020 +0200 2 files changed, 44 insertions(+), 5 deletions(-) ✔ Patches applied There are 9 commits in the PR. Attempting autorebase. Rebasing (2/18)
PR-URL: #36070 |
- Add support - Add test - Docs once PR is up PR-URL: #36070 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
benjamingr commented Nov 21, 2020
Landed manually in 630afc3 🎉 |
- Add support - Add test - Docs once PR is up PR-URL: #36070 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
targos added backport-open-v14.x and removed commit-queue-failed 
- Add support - Add test - Docs once PR is up PR-URL: nodejs#36070 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
- Add support - Add test - Docs once PR is up PR-URL: nodejs#36070 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
- Add support - Add test - Docs once PR is up PR-URL: nodejs#36070 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
- Add support - Add test - Docs once PR is up PR-URL: #36070 Backport-PR-URL: #38386 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
7 participants
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes