Skip to content
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: add support for AbortSignal to http2Session.request #36070

Closed
9 changes: 9 additions & 0 deletions doc/api/http2.md
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
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
Member

Choose a reason for hiding this comment

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

What happens if this is aborted already?

Copy link
Contributor Author

@MadaraUchiha MadaraUchiha Nov 19, 2020

Choose a reason for hiding this comment

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

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
Member

Choose a reason for hiding this comment

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

Yes, it would need to abort immediately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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
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()));
}));
}