From 1960f9c82117896d51cf77450b1cecac3d57bf97 Mon Sep 17 00:00:00 2001 From: Connor Prussin Date: Tue, 26 Mar 2019 15:34:11 -0700 Subject: [PATCH 1/3] Pass a next() to the uncaught exception handler --- lib/server.js | 11 ++++++++++- test/server.test.js | 28 ++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/lib/server.js b/lib/server.js index e70f012b4..5570d51a9 100644 --- a/lib/server.js +++ b/lib/server.js @@ -1405,7 +1405,16 @@ Server.prototype._routeErrorResponse = function _routeErrorResponse( self.handleUncaughtExceptions && self.listenerCount('uncaughtException') > 1 ) { - self.emit('uncaughtException', req, res, req.route, err); + self.emit( + 'uncaughtException', + req, + res, + req.route, + err, + function uncaughtExceptionCompleted() { + self._finishReqResCycle(req, res, err); + } + ); return; } diff --git a/test/server.test.js b/test/server.test.js index ba19cbb55..2683b2517 100644 --- a/test/server.test.js +++ b/test/server.test.js @@ -2051,6 +2051,34 @@ test( } ); +// eslint-disable-next-line max-len +test("should emit 'after' on client closed request with an uncaughtException", function(t) { + SERVER.on('after', function(req, res, route, err) { + t.ok(err); + t.equal(req.connectionState(), 'close'); + t.equal(res.statusCode, 444); + t.equal(err.name, 'Error'); + t.end(); + }); + + SERVER.on('uncaughtException', function(req, res, route, err, next) { + next(); + }); + + SERVER.get('/foobar', function(req, res, next) { + // fast client times out at 500ms, wait for 800ms which should cause + // client to timeout + setTimeout(function() { + throw new Error('foo'); + }, 800); + }); + + FAST_CLIENT.get('/foobar', function(err, _, res) { + t.ok(err); + t.equal(err.name, 'RequestTimeoutError'); + }); +}); + test('should increment/decrement inflight request count', function(t) { SERVER.get('/foo', function(req, res, next) { t.equal(SERVER.inflightRequests(), 1); From 435e8c7211d3b2b5c2e025f5d2d2da123f978d27 Mon Sep 17 00:00:00 2001 From: Julien Gilli Date: Fri, 5 Apr 2019 14:54:43 -0700 Subject: [PATCH 2/3] make test more reliable and add documentation --- docs/api/server-events.md | 14 ++++++++++---- test/server.test.js | 35 ++++++++++++++++++++++++++--------- 2 files changed, 36 insertions(+), 13 deletions(-) diff --git a/docs/api/server-events.md b/docs/api/server-events.md index c3b183bca..48c29ded8 100644 --- a/docs/api/server-events.md +++ b/docs/api/server-events.md @@ -107,15 +107,21 @@ server.get('/', function(req, res, next) { return next(); }); -server.on('uncaughtException', function(req, res, route, err) { +server.on('uncaughtException', function(req, res, route, err, callback) { // this event will be fired, with the error object from above: // ReferenceError: x is not defined + res.send(504, 'boom'); + callback(); }); ``` -If you listen to this event, you __must__ send a response to the client. This -behavior is different from the standard error events. If you do not listen to -this event, restify's default behavior is to call `res.send()` with the error +If you listen to this event, you __must__: + +1. send a response to the client _and_ +2. call the callback function passed as the fourth argument of the event listener + +This behavior is different from the standard error events. If you do not listen +to this event, restify's default behavior is to call `res.send()` with the error that was thrown. The signature is for the after event is as follows: diff --git a/test/server.test.js b/test/server.test.js index 2683b2517..62c25927b 100644 --- a/test/server.test.js +++ b/test/server.test.js @@ -2051,31 +2051,48 @@ test( } ); +// This test reproduces https://github.com/restify/node-restify/issues/1765. It +// specifically tests the edge case of an exception being thrown from a route +// handler _after_ the response is considered to be "flushed" (for instance when +// the request is aborted before a response is sent and an exception is thrown). // eslint-disable-next-line max-len -test("should emit 'after' on client closed request with an uncaughtException", function(t) { +test("should emit 'after' on uncaughtException after response closed with custom uncaughtException listener", function(t) { + var ERR_MSG = 'foo'; + var gotAfter = false; + var gotReqCallback = false; + SERVER.on('after', function(req, res, route, err) { + gotAfter = true; t.ok(err); t.equal(req.connectionState(), 'close'); t.equal(res.statusCode, 444); t.equal(err.name, 'Error'); - t.end(); + t.equal(err.message, ERR_MSG); + if (gotReqCallback) { + t.end(); + } }); - SERVER.on('uncaughtException', function(req, res, route, err, next) { - next(); + SERVER.on('uncaughtException', function(req, res, route, err, callback) { + callback(); }); SERVER.get('/foobar', function(req, res, next) { - // fast client times out at 500ms, wait for 800ms which should cause - // client to timeout - setTimeout(function() { - throw new Error('foo'); - }, 800); + res.on('close', function onResClose() { + // We throw this error in the response's close event handler on + // purpose to exercise the code path where we mark the route + // handlers as finished _after_ the response is marked as flushed. + throw new Error(ERR_MSG); + }); }); FAST_CLIENT.get('/foobar', function(err, _, res) { + gotReqCallback = true; t.ok(err); t.equal(err.name, 'RequestTimeoutError'); + if (gotAfter) { + t.end(); + } }); }); From 565d6f39157f0eff6dbe060ffa64ac6b4aa41ed1 Mon Sep 17 00:00:00 2001 From: Julien Gilli Date: Fri, 5 Apr 2019 17:13:03 -0700 Subject: [PATCH 3/3] add comments --- lib/server.js | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/lib/server.js b/lib/server.js index 5570d51a9..d3da14442 100644 --- a/lib/server.js +++ b/lib/server.js @@ -1412,6 +1412,16 @@ Server.prototype._routeErrorResponse = function _routeErrorResponse( req.route, err, function uncaughtExceptionCompleted() { + // We provide a callback to listeners of the 'uncaughtException' + // event and we call _finishReqResCycle when that callback is + // called so that, in case the actual request/response lifecycle + // was completed _before_ the error was thrown or emitted, and + // thus _before_ route handlers were marked as "finished", we + // can still mark the req/res lifecycle as complete. + // This edge case can occur when e.g. a client aborts a request + // and the route handler that handles that request throws an + // uncaught exception _after_ the request was aborted and the + // response was closed. self._finishReqResCycle(req, res, err); } );