http2: add support for AbortSignal to http2Session.request - #36070
http2: add support for AbortSignal to http2Session.request#36070MadaraUchiha wants to merge 9 commits into
Conversation
Sorry, something went wrong.
|
Review requested:
|
Sorry, something went wrong.
|
@aduh95 this is HTTP2 and the one I opened is HTTP :) I spoke with Dor and suggested this as a contribution opportunity |
Sorry, something went wrong.
|
Ping @jasnell on http2 + AbortController :] |
Sorry, something went wrong.
Sorry, something went wrong.
There was a problem hiding this comment.
I would prefer this to be called with an error.
Sorry, something went wrong.
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
Sorry, something went wrong.
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?)
Sorry, something went wrong.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
@jasnell but wouldn't the request get aborted with an ECONNRESET (like in the test) anyway?
Sorry, something went wrong.
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?)
Sorry, something went wrong.
There was a problem hiding this comment.
Test appreciated, I think the errors a refine no?
Sorry, something went wrong.
Sorry, something went wrong.
|
See additional discussion here: #36048 (comment) |
Sorry, something went wrong.
|
This needs a rebase. |
Sorry, something went wrong.
- 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
Sorry, something went wrong.
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
|
Sorry, something went wrong.
- Add support - Add test - Docs once PR is up PR-URL: #36070 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
- Add support - Add test - Docs once PR is up PR-URL: #36070 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
- Add support - Add test - Docs once PR is up PR-URL: nodejs#36070 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Checklist