From 0da821920f735a1b9b194d0b3ddcde360a0f77f3 Mon Sep 17 00:00:00 2001 From: Peter Marton Date: Sat, 18 Nov 2017 09:23:23 +0100 Subject: [PATCH] feat(server): remove aborted event handling, always listen for close BREAKING CHANGE: server always throws RequestCloseError instead of RequestAbortedError --- docs/guides/6to7guide.md | 7 ++++++ lib/errorTypes.js | 1 - lib/plugins/metrics.js | 12 +++++----- lib/plugins/static.js | 6 +---- lib/request.js | 10 +++----- lib/server.js | 44 ++++++++++++++++-------------------- test/plugins/metrics.test.js | 44 +++--------------------------------- test/server.test.js | 28 ----------------------- 8 files changed, 39 insertions(+), 113 deletions(-) diff --git a/docs/guides/6to7guide.md b/docs/guides/6to7guide.md index 8bff98891..0b2f38873 100644 --- a/docs/guides/6to7guide.md +++ b/docs/guides/6to7guide.md @@ -13,6 +13,13 @@ backend. ## Breaking Changes +### Server throws `RequestCloseError` instead of `RequestAbortedError` + +Server throws `RequestCloseError` instead of `RequestAbortedError` in the case +of the request was terminated by the client for some reason. + +The new version of restify never throws `RequestAbortedError`. + ### `req.params` property is not available in `use` and `pre` `req.params` is only available in route handlers and in `after` handlers. diff --git a/lib/errorTypes.js b/lib/errorTypes.js index a941e6b5c..e31e22cb7 100644 --- a/lib/errorTypes.js +++ b/lib/errorTypes.js @@ -3,4 +3,3 @@ var errors = require('restify-errors'); errors.makeConstructor('RequestCloseError'); -errors.makeConstructor('RequestAbortedError'); diff --git a/lib/plugins/metrics.js b/lib/plugins/metrics.js index 2bb462beb..f5000e17e 100644 --- a/lib/plugins/metrics.js +++ b/lib/plugins/metrics.js @@ -44,10 +44,10 @@ function createMetrics(opts, callback) { // e.g., /foo?a=1 => /foo path: req.path(), // connection state can currently only have the following values: - // 'close' | 'aborted' | undefined. + // 'close' | undefined. // // it is possible to get a 200 statusCode with a connectionState - // value of 'close' or 'aborted'. i.e., the client timed out, + // value of 'close'. i.e., the client timed out, // but restify thinks it "sent" a response. connectionState should // always be the primary source of truth here, and check it first // before consuming statusCode. otherwise, it may result in skewed @@ -76,11 +76,11 @@ function createMetrics(opts, callback) { * @param {Number} metrics.inflightRequests Number of inflight requests pending * in restify. * @param {Number} metrics.unifinishedRequests Same as `inflightRequests` - * @param {String} metrics.connectionState can be either `'close'`, - * `'aborted'`, or `undefined`. If this value is set, err will be a - * corresponding `RequestCloseError` or `RequestAbortedError`. + * @param {String} metrics.connectionState can be either `'close'` or + * `undefined`. If this value is set, err will be a + * corresponding `RequestCloseError`. * If connectionState is either - * `'close'` or `'aborted'`, then the `statusCode` is not applicable since the + * `'close'`, then the `statusCode` is not applicable since the * connection was severed before a response was written. * @param {Request} req the request obj * @param {Response} res the response obj diff --git a/lib/plugins/static.js b/lib/plugins/static.js index fa6fff58d..2470ad214 100644 --- a/lib/plugins/static.js +++ b/lib/plugins/static.js @@ -88,11 +88,7 @@ function serveStatic(options) { var re = new RegExp('^' + escapeRE(p) + '/?.*'); function serveFileFromStats(file, err, stats, isGzip, req, res, next) { - if ( - typeof req.connectionState === 'function' && - (req.connectionState() === 'close' || - req.connectionState() === 'aborted') - ) { + if (typeof req.closed === 'function' && req.closed()) { next(false); return; } diff --git a/lib/request.js b/lib/request.js index 10e317e9e..9ebf4a1db 100644 --- a/lib/request.js +++ b/lib/request.js @@ -827,13 +827,12 @@ function patch(Request) { /** * Returns the connection state of the request. Current possible values are: * - `close` - when the request has been closed by the clien - * - `aborted` - when the socket was closed unexpectedly * * @public * @memberof Request * @instance * @function connectionState - * @returns {String} connection state (`"close"`, `"aborted"`) + * @returns {String} connection state (`"close"`) */ Request.prototype.connectionState = function connectionState() { var self = this; @@ -841,7 +840,7 @@ function patch(Request) { }; /** - * Returns true when connection state is "close" or "aborted" + * Returns true when connection state is "close" * * @private * @memberof Request @@ -851,10 +850,7 @@ function patch(Request) { */ Request.prototype.closed = function closed() { var self = this; - return ( - self._connectionState === 'close' || - self._connectionState === 'aborted' - ); + return self.connectionState() === 'close'; }; /** diff --git a/lib/server.js b/lib/server.js index 86223f253..ea3bfcd9b 100644 --- a/lib/server.js +++ b/lib/server.js @@ -1017,24 +1017,31 @@ Server.prototype._setupRequest = function _setupRequest(req, res) { } // Request lifecycle events - // attach a listener for 'close' and 'aborted' events, this will let us set + // attach a listener for 'close' events, this will let us set // a flag so that we can stop processing the request if the client closes // the connection (or we lose the connection). - req.once('close', function onceClose() { + function onClose() { + res.removeListener('finish', onFinish); + res.removeListener('error', onError); req._connectionState = 'close'; - self._finishReqResCycle(req, res); - }); - req.once('aborted', function onceAborted() { - req._connectionState = 'aborted'; - }); + + self._finishReqResCycle(req, res, new errors.RequestCloseError()); + } + req.once('close', onClose); // Response lifecycle events - res.once('finish', function onceFinish() { + function onFinish() { + req.removeListener('close', onClose); + res.removeListener('error', onError); self._finishReqResCycle(req, res, res.err); - }); - res.once('error', function onceError(err) { + } + function onError(err) { + req.removeListener('close', onClose); + res.removeListener('finish', onFinish); self._finishReqResCycle(req, res, err); - }); + } + res.once('finish', onFinish); + res.once('error', onError); // attach a listener for the response's 'redirect' event res.on('redirect', function onRedirect(redirectLocation) { @@ -1077,20 +1084,7 @@ Server.prototype._finishReqResCycle = function _finishReqResCycle( self.emit('after', req, res, route, err || res.formatterErr); }); } else { - // if there was an error in the processing of that request, use it. - // if not, check to see if the request was closed or aborted early and - // create an error out of that for audit logging. - var afterErr = err; - - if (!afterErr) { - if (req._connectionState === 'close') { - afterErr = new errors.RequestCloseError(); - } else if (req._connectionState === 'aborted') { - afterErr = new errors.RequestAbortedError(); - } - } - - self.emit('after', req, res, route, afterErr); + self.emit('after', req, res, route, err); } // decrement number of requests diff --git a/test/plugins/metrics.test.js b/test/plugins/metrics.test.js index 2d81b1218..5efb2eade 100644 --- a/test/plugins/metrics.test.js +++ b/test/plugins/metrics.test.js @@ -80,7 +80,7 @@ describe('request metrics plugin', function() { }); }); - it.skip("should return 'RequestCloseError' err", function(done) { + it("should return 'RequestCloseError' err", function(done) { // we test that the client times out and closes the request. server // flushes request responsibly but connectionState should indicate it // was closed by the server. @@ -96,7 +96,7 @@ describe('request metrics plugin', function() { assert.equal(err.name, 'RequestCloseError'); assert.isObject(metrics, 'metrics'); - assert.equal(metrics.statusCode, 202); + assert.equal(metrics.statusCode, 200); // router doesn't run assert.isAtLeast(metrics.latency, 200); assert.equal(metrics.path, '/foo'); assert.equal(metrics.method, 'GET'); @@ -109,6 +109,7 @@ describe('request metrics plugin', function() { SERVER.get('/foo', function(req, res, next) { setTimeout(function() { + assert.fail('Client has already closed request'); res.send(202, 'hello world'); return next(); }, 250); @@ -121,45 +122,6 @@ describe('request metrics plugin', function() { }); }); - it("should return 'RequestAbortedError' err", function(done) { - // we test that the client times out and closes the request. server - // flushes request responsibly but connectionState should indicate it - // was closed by the server. - - SERVER.on( - 'after', - restify.plugins.metrics( - { - server: SERVER - }, - function(err, metrics, req, res, route) { - assert.ok(err); - assert.equal(err.name, 'RequestAbortedError'); - - assert.isObject(metrics, 'metrics'); - assert.equal(metrics.statusCode, 202); - assert.isAtLeast(metrics.latency, 200); - assert.equal(metrics.path, '/foo'); - assert.equal(metrics.method, 'GET'); - assert.equal(metrics.connectionState, 'aborted'); - assert.isNumber(metrics.inflightRequests); - } - ) - ); - - SERVER.get('/foo', function(req, res, next) { - // simulate request being aborted by TCP socket being closed - req.emit('aborted'); - res.send(202, 'hello world'); - return next(); - }); - - CLIENT.get('/foo?a=1', function(err, _, res) { - assert.ifError(err); - return done(); - }); - }); - it('should handle uncaught exceptions', function(done) { // we test that the client times out and closes the request. server // flushes request responsibly but connectionState should indicate it diff --git a/test/server.test.js b/test/server.test.js index 6e1fae76a..fb8c1a01a 100644 --- a/test/server.test.js +++ b/test/server.test.js @@ -1817,34 +1817,6 @@ test( } ); -// TODO: what's the difference compared to "close"? -// test( -// "should 'emit' after on aborted request " + -// "(req.connectionState(): 'aborted')", -// function(t) { -// SERVER.on('after', function(req, res, route, err) { -// t.ok(err); -// t.equal(req.connectionState(), 'aborted'); -// t.equal(err.name, 'RequestAbortedError'); -// }); -// -// SERVER.get('/foobar', function(req, res, next) { -// req.emit('aborted'); -// // fast client times out at 500ms, wait for 800ms which should cause -// // client to timeout -// setTimeout(function() { -// return next(); -// }, 800); -// }); -// -// FAST_CLIENT.get('/foobar', function(err, _, res) { -// t.ok(err); -// t.equal(err.name, 'RequestTimeoutError'); -// t.end(); -// }); -// } -// ); - test('should increment/decrement inflight request count', function(t) { SERVER.get('/foo', function(req, res, next) { t.equal(SERVER.inflightRequests(), 1);