FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

http2: add support for AbortSignal to http2Session.request by MadaraUchiha · Pull Request #36070 · nodejs/node · GitHub

/ node Public
9 changes: 9 additions & 0 deletions doc/api/http2.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@
<!-- YAML
added: v8.4.0
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/36070
description: It is possible to abort a request with an AbortSignal.
- version: v15.0.0
pr-url: https://github.com/nodejs/node/pull/34664
description: Requests with the `host` header (with or without
Expand Down Expand Up @@ -846,6 +849,8 @@ added: v8.4.0
and `256` (inclusive).
* `waitForTrailers` {boolean} When `true`, the `Http2Stream` will emit the
`'wantTrailers'` event after the final `DATA` frame has been sent.
* `signal` {AbortSignal} An AbortSignal that may be used to abort an ongoing
request.

* Returns: {ClientHttp2Stream}

Expand Down Expand Up @@ -882,6 +887,10 @@ close when the final `DATA` frame is transmitted. User code must call either
`http2stream.sendTrailers()` or `http2stream.close()` to close the
`Http2Stream`.

When `options.signal` is set with an `AbortSignal` and then `abort` on the
corresponding `AbortController` is called, the request will emit an `'error'`
event with an `AbortError` error.

The `:method` and `:path` pseudo-headers are not specified within `headers`,
they respectively default to:

Expand Down
18 changes: 17 additions & 1 deletion lib/internal/http2/core.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,8 @@ const {
ERR_OUT_OF_RANGE,
ERR_SOCKET_CLOSED
},
hideStackFrames
hideStackFrames,
AbortError
} = require('internal/errors');
const {
isUint32,
Expand All @@ -118,6 +119,7 @@ const {
validateNumber,
validateString,
validateUint32,
validateAbortSignal,
} = require('internal/validators');
const fsPromisesInternal = require('internal/fs/promises');
const { utcDate } = require('internal/http');
Expand Down Expand Up @@ -1721,6 +1723,20 @@ class ClientHttp2Session extends Http2Session {
if (options.waitForTrailers)
stream[kState].flags |= STREAM_FLAGS_HAS_TRAILERS;

const { signal } = options;
if (signal) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

What happens if this is aborted already?

MadaraUchiha Nov 19, 2020
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

That's an excellent question.

Looking at the DOM implementation, it appears like the expected behavior is to abort immediately. I shall add this (and a test) after my 🍕 :D

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yes, it would need to abort immediately.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@mcollina Done, PTALA.

Also @benjamingr this is likely something we'd want to do in http_client as well.

validateAbortSignal(signal, 'options.signal');
const aborter = () => stream.destroy(new AbortError());
if (signal.aborted) {
aborter();
} else {
signal.addEventListener('abort', aborter);
stream.once('close', () => {
signal.removeEventListener('abort', aborter);
});
}
}

const onConnect = FunctionPrototypeBind(requestOnConnect,
stream, headersList, options);
if (this.connecting) {
Expand Down
74 changes: 74 additions & 0 deletions test/parallel/test-http2-client-destroy.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ if (!common.hasCrypto)
const assert = require('assert');
const h2 = require('http2');
const { kSocket } = require('internal/http2/util');
const { kEvents } = require('internal/event_target');
const Countdown = require('../common/countdown');

{
Expand Down Expand Up @@ -167,3 +168,76 @@ const Countdown = require('../common/countdown');
req.on('close', common.mustCall(() => server.close()));
}));
}

// Destroy with AbortSignal
{
const server = h2.createServer();
const controller = new AbortController();

server.on('stream', common.mustNotCall());
server.listen(0, common.mustCall(() => {
const client = h2.connect(`http://localhost:${server.address().port}`);
client.on('close', common.mustCall());

const { signal } = controller;
assert.strictEqual(signal[kEvents].get('abort'), undefined);

client.on('error', common.mustCall(() => {
// After underlying stream dies, signal listener detached
assert.strictEqual(signal[kEvents].get('abort'), undefined);
}));

const req = client.request({}, { signal });

req.on('error', common.mustCall((err) => {
assert.strictEqual(err.code, 'ABORT_ERR');
assert.strictEqual(err.name, 'AbortError');
}));
req.on('close', common.mustCall(() => server.close()));

assert.strictEqual(req.aborted, false);
assert.strictEqual(req.destroyed, false);
// Signal listener attached
assert.strictEqual(signal[kEvents].get('abort').size, 1);

controller.abort();

assert.strictEqual(req.aborted, false);
assert.strictEqual(req.destroyed, true);
}));
}
// Pass an already destroyed signal to abort immediately.
{
const server = h2.createServer();
const controller = new AbortController();

server.on('stream', common.mustNotCall());
server.listen(0, common.mustCall(() => {
const client = h2.connect(`http://localhost:${server.address().port}`);
client.on('close', common.mustCall());

const { signal } = controller;
controller.abort();

assert.strictEqual(signal[kEvents].get('abort'), undefined);

client.on('error', common.mustCall(() => {
// After underlying stream dies, signal listener detached
assert.strictEqual(signal[kEvents].get('abort'), undefined);
}));

const req = client.request({}, { signal });
// Signal already aborted, so no event listener attached.
assert.strictEqual(signal[kEvents].get('abort'), undefined);

assert.strictEqual(req.aborted, false);
// Destroyed on same tick as request made
assert.strictEqual(req.destroyed, true);

req.on('error', common.mustCall((err) => {
assert.strictEqual(err.code, 'ABORT_ERR');
assert.strictEqual(err.name, 'AbortError');
}));
req.on('close', common.mustCall(() => server.close()));
}));
}

Back | FazBrowse Home | New Git URL