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: refactor error handling #14991

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 35 additions & 4 deletions doc/api/http2.md
Original file line number Diff line number Diff line change
Expand Up @@ -1118,6 +1118,8 @@ added: v8.4.0
* `headers` {[Headers Object][]}
* `options` {Object}
* `statCheck` {Function}
* `onError` {Function} Callback function invoked in the case of an
Error before send
* `getTrailers` {Function} Callback function invoked to collect trailer
headers.
* `offset` {number} The offset position at which to begin reading
Expand Down Expand Up @@ -1146,6 +1148,16 @@ server.on('stream', (stream) => {
function statCheck(stat, headers) {
headers['last-modified'] = stat.mtime.toUTCString();
}

function onError(err) {
if (err.code === 'ENOENT') {
stream.respond({ ':status': 404 });
} else {
stream.respond({ ':status': 500 });
}
stream.end();
}

stream.respondWithFile('/some/file',
{ 'content-type': 'text/plain' },
{ statCheck });
Expand Down Expand Up @@ -1178,6 +1190,10 @@ The `offset` and `length` options may be used to limit the response to a
specific range subset. This can be used, for instance, to support HTTP Range
requests.

The `options.onError` function may also be used to handle all the errors
that could happen before the delivery of the file is initiated. The
default behavior is to destroy the stream.

When set, the `options.getTrailers()` function is called immediately after
queuing the last chunk of payload data to be sent. The callback is passed a
single object (with a `null` prototype) that the listener may used to specify
Expand Down Expand Up @@ -1208,6 +1224,19 @@ added: v8.4.0

* Extends: {net.Server}

In `Http2Server`, there is no `'clientError'` event as there is in
HTTP1. However, there are `'socketError'`, `'sessionError'`, and
`'streamError'`, for error happened on the socket, session or stream
respectively.

#### Event: 'socketError'
<!-- YAML
added: v8.4.0
-->

The `'socketError'` event is emitted when a `'socketError'` event is emitted by
an `Http2Session` associated with the server.

#### Event: 'sessionError'
<!-- YAML
added: v8.4.0
Expand All @@ -1217,13 +1246,15 @@ The `'sessionError'` event is emitted when an `'error'` event is emitted by
an `Http2Session` object. If no listener is registered for this event, an
`'error'` event is emitted.

#### Event: 'socketError'
#### Event: 'streamError'
<!-- YAML
added: v8.4.0
added: REPLACEME
-->

The `'socketError'` event is emitted when a `'socketError'` event is emitted by
an `Http2Session` associated with the server.
* `socket` {http2.ServerHttp2Stream}

If an `ServerHttp2Stream` emits an `'error'` event, it will be forwarded here.
The stream will already be destroyed when this event is triggered.

#### Event: 'stream'
<!-- YAML
Expand Down
4 changes: 2 additions & 2 deletions lib/http2.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ const {
createSecureServer,
connect,
Http2ServerRequest,
Http2ServerResponse,
Http2ServerResponse
} = require('internal/http2/core');

module.exports = {
Expand All @@ -27,5 +27,5 @@ module.exports = {
createSecureServer,
connect,
Http2ServerResponse,
Http2ServerRequest,
Http2ServerRequest
};
16 changes: 7 additions & 9 deletions lib/internal/http2/compat.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,13 @@ function onStreamEnd() {
}

function onStreamError(error) {
const request = this[kRequest];
request.emit('error', error);
// this is purposefully left blank
//
// errors in compatibility mode are
// not forwarded to the request
// and response objects. However,
// they are forwarded to 'clientError'
// on the server by Http2Stream
}

function onRequestPause() {
Expand All @@ -82,11 +87,6 @@ function onStreamResponseDrain() {
response.emit('drain');
}

function onStreamResponseError(error) {
const response = this[kResponse];
response.emit('error', error);
}

function onStreamClosedRequest() {
const req = this[kRequest];
req.push(null);
Expand Down Expand Up @@ -271,9 +271,7 @@ class Http2ServerResponse extends Stream {
stream[kResponse] = this;
this.writable = true;
stream.on('drain', onStreamResponseDrain);
stream.on('error', onStreamResponseError);
stream.on('close', onStreamClosedResponse);
stream.on('aborted', onAborted.bind(this));
const onfinish = this[kFinish].bind(this);
stream.on('streamClosed', onfinish);
stream.on('finish', onfinish);
Expand Down
43 changes: 37 additions & 6 deletions lib/internal/http2/core.js
Original file line number Diff line number Diff line change
Expand Up @@ -1506,6 +1506,13 @@ class Http2Stream extends Duplex {
this.once('ready', this._destroy.bind(this, err, callback));
return;
}

const server = session[kServer];

if (err && server) {
server.emit('streamError', err, this);
}

process.nextTick(() => {
debug(`[${sessionName(session[kType])}] destroying stream ${this[kID]}`);

Expand All @@ -1529,9 +1536,8 @@ class Http2Stream extends Duplex {
// All done
const rst = this[kState].rst;
const code = rst ? this[kState].rstCode : NGHTTP2_NO_ERROR;
if (code !== NGHTTP2_NO_ERROR) {
const err = new errors.Error('ERR_HTTP2_STREAM_ERROR', code);
process.nextTick(() => this.emit('error', err));
if (!err && code !== NGHTTP2_NO_ERROR) {
err = new errors.Error('ERR_HTTP2_STREAM_ERROR', code);
}
process.nextTick(emit.bind(this, 'streamClosed', code));
debug(`[${sessionName(session[kType])}] stream ${this[kID]} destroyed`);
Expand Down Expand Up @@ -1634,13 +1640,24 @@ function doSendFileFD(session, options, fd, headers, getTrailers, err, stat) {
abort(this);
return;
}
const onError = options.onError;

if (err) {
process.nextTick(() => this.emit('error', err));
if (onError) {
onError(err);
} else {
this.destroy(err);
}
return;
}

if (!stat.isFile()) {
err = new errors.Error('ERR_HTTP2_SEND_FILE');
process.nextTick(() => this.emit('error', err));
if (onError) {
onError(err);
} else {
this.destroy(err);
}
return;
}

Expand Down Expand Up @@ -1677,12 +1694,17 @@ function doSendFileFD(session, options, fd, headers, getTrailers, err, stat) {

function afterOpen(session, options, headers, getTrailers, err, fd) {
const state = this[kState];
const onError = options.onError;
if (this.destroyed || session.destroyed) {
abort(this);
return;
}
if (err) {
process.nextTick(() => this.emit('error', err));
if (onError) {
onError(err);
} else {
this.destroy(err);
}
return;
}
state.fd = fd;
Expand All @@ -1691,13 +1713,20 @@ function afterOpen(session, options, headers, getTrailers, err, fd) {
doSendFileFD.bind(this, session, options, fd, headers, getTrailers));
}

function streamOnError(err) {
// we swallow the error for parity with HTTP1
// all the errors that ends here are not critical for the project
debug('ServerHttp2Stream errored, avoiding uncaughtException', err);
}


class ServerHttp2Stream extends Http2Stream {
constructor(session, id, options, headers) {
super(session, options);
this[kInit](id);
this[kProtocol] = headers[HTTP2_HEADER_SCHEME];
this[kAuthority] = headers[HTTP2_HEADER_AUTHORITY];
this.on('error', streamOnError);
debug(`[${sessionName(session[kType])}] created serverhttp2stream`);
}

Expand Down Expand Up @@ -2556,6 +2585,8 @@ module.exports = {
createServer,
createSecureServer,
connect,
Http2Session,
Http2Stream,
Http2ServerRequest,
Http2ServerResponse
};
Expand Down
17 changes: 5 additions & 12 deletions test/parallel/test-http2-client-stream-destroy-before-connect.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,18 +36,11 @@ server.on('listening', common.mustCall(() => {
req.destroy(err);

req.on('error', common.mustCall((err) => {
const fn = err.code === 'ERR_HTTP2_STREAM_ERROR' ?
common.expectsError({
code: 'ERR_HTTP2_STREAM_ERROR',
type: Error,
message: 'Stream closed with error code 2'
}) :
common.expectsError({
type: Error,
message: 'test'
});
fn(err);
}, 2));
common.expectsError({
type: Error,
message: 'test'
})(err);
}));

req.on('streamClosed', common.mustCall((code) => {
assert.strictEqual(req.rstCode, NGHTTP2_INTERNAL_ERROR);
Expand Down
52 changes: 52 additions & 0 deletions test/parallel/test-http2-compat-errors.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
// Flags: --expose-http2 --expose-internals
'use strict';

const common = require('../common');
if (!common.hasCrypto)
common.skip('missing crypto');
const assert = require('assert');
const h2 = require('http2');
const { Http2Stream } = require('internal/http2/core');

// Errors should not be reported both in Http2ServerRequest
// and Http2ServerResponse

let expected = null;

const server = h2.createServer(common.mustCall(function(req, res) {
req.on('error', common.mustNotCall());
res.on('error', common.mustNotCall());
req.on('aborted', common.mustCall());
res.on('aborted', common.mustNotCall());

res.write('hello');

expected = new Error('kaboom');
res.stream.destroy(expected);
server.close();
}));

server.on('streamError', common.mustCall(function(err, stream) {
assert.strictEqual(err, expected);
assert.strictEqual(stream instanceof Http2Stream, true);
}));

server.listen(0, common.mustCall(function() {
const port = server.address().port;

const url = `http://localhost:${port}`;
const client = h2.connect(url, common.mustCall(function() {
const headers = {
':path': '/foobar',
':method': 'GET',
':scheme': 'http',
':authority': `localhost:${port}`,
};
const request = client.request(headers);
request.on('data', common.mustCall(function(chunk) {
// cause an error on the server side
client.destroy();
}));
request.end();
}));
}));
48 changes: 48 additions & 0 deletions test/parallel/test-http2-respond-file-404.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
// Flags: --expose-http2
'use strict';

const common = require('../common');
if (!common.hasCrypto)
common.skip('missing crypto');
const http2 = require('http2');
const assert = require('assert');
const path = require('path');

const {
HTTP2_HEADER_CONTENT_TYPE
} = http2.constants;

const server = http2.createServer();
server.on('stream', (stream) => {
const file = path.join(process.cwd(), 'not-a-file');
stream.respondWithFile(file, {
[HTTP2_HEADER_CONTENT_TYPE]: 'text/plain'
}, {
onError(err) {
common.expectsError({
code: 'ENOENT',
type: Error,
message: `ENOENT: no such file or directory, open '${file}'`
})(err);

stream.respond({ ':status': 404 });
stream.end();
},
statCheck: common.mustNotCall()
});
});
server.listen(0, () => {

const client = http2.connect(`http://localhost:${server.address().port}`);
const req = client.request();

req.on('response', common.mustCall((headers) => {
assert.strictEqual(headers[':status'], 404);
}));
req.on('data', common.mustNotCall());
req.on('end', common.mustCall(() => {
client.destroy();
server.close();
}));
req.end();
});
Loading