From 2857f155825ab6eb329a8c0d6a733c059fee1fd7 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Thu, 24 Aug 2017 16:11:55 +0200 Subject: [PATCH] http2: respondWith* to the new error handling Fixes: https://github.com/nodejs/node/issues/14963 --- doc/api/http2.md | 16 +++++++ lib/internal/http2/core.js | 22 +++++++-- test/parallel/test-http2-respond-file-404.js | 48 +++++++++++++++++++ .../test-http2-respond-file-error-dir.js | 46 ++++++++++++++++++ 4 files changed, 129 insertions(+), 3 deletions(-) create mode 100644 test/parallel/test-http2-respond-file-404.js create mode 100644 test/parallel/test-http2-respond-file-error-dir.js diff --git a/doc/api/http2.md b/doc/api/http2.md index 450de73f69823b..57e1b74e7e9edc 100755 --- a/doc/api/http2.md +++ b/doc/api/http2.md @@ -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 @@ -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 }); @@ -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 diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index f8f3942cc5d01c..d45acb29f1f699 100755 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -1640,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; } @@ -1683,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; diff --git a/test/parallel/test-http2-respond-file-404.js b/test/parallel/test-http2-respond-file-404.js new file mode 100644 index 00000000000000..b8e2a39c9a2c88 --- /dev/null +++ b/test/parallel/test-http2-respond-file-404.js @@ -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(); +}); diff --git a/test/parallel/test-http2-respond-file-error-dir.js b/test/parallel/test-http2-respond-file-error-dir.js new file mode 100644 index 00000000000000..fd212a4c339dc1 --- /dev/null +++ b/test/parallel/test-http2-respond-file-error-dir.js @@ -0,0 +1,46 @@ +// 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 { + HTTP2_HEADER_CONTENT_TYPE +} = http2.constants; + +const server = http2.createServer(); +server.on('stream', (stream) => { + stream.respondWithFile(process.cwd(), { + [HTTP2_HEADER_CONTENT_TYPE]: 'text/plain' + }, { + onError(err) { + common.expectsError({ + code: 'ERR_HTTP2_SEND_FILE', + type: Error, + message: 'Only regular files can be sent' + })(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(); +});