http2: add support for AbortSignal to http2Session.request #36070
Conversation
|
Review requested:
|
|
@aduh95 this is HTTP2 and the one I opened is HTTP :) I spoke with Dor and suggested this as a contribution opportunity |
|
Ping @jasnell on http2 + AbortController :] |
github-actions
bot
removed
the
request-ci
lib/internal/http2/core.js
Outdated
| const signal = options.signal; | ||
| if (signal) { | ||
| validateAbortSignal(signal, 'options.signal'); | ||
| const listener = () => stream.destroy(); |
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-actions
bot
removed
the
request-ci
MadaraUchiha
force-pushed
the
add-abortsignal-http2-request
branch
from
3875bfb
to
dc97603
Compare
|
See additional discussion here: #36048 (comment) |
|
This needs a rebase. |
MadaraUchiha
force-pushed
the
add-abortsignal-http2-request
branch
from
dc97603
to
db9ad63
Compare
- 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
MadaraUchiha
force-pushed
the
add-abortsignal-http2-request
branch
from
fe2b00e
to
9aca7c4
Compare
github-actions
bot
removed
the
request-ci
benjamingr
added
the
commit-queue
github-actions
bot
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]>
|
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]>
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes