From 5e8b5e2b28e32c79c413d9dec2466fe8f1135332 Mon Sep 17 00:00:00 2001
From: Connor Prussin <connor@prussin.net>
Date: Thu, 11 Apr 2019 11:17:55 -0700
Subject: [PATCH] feat: provide callback to uncaughtException handler (#1766)

---
 docs/api/server-events.md | 14 ++++++++----
 lib/server.js             | 21 +++++++++++++++++-
 test/server.test.js       | 45 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 75 insertions(+), 5 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/lib/server.js b/lib/server.js
index e70f012b4..d3da14442 100644
--- a/lib/server.js
+++ b/lib/server.js
@@ -1405,7 +1405,26 @@ 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() {
+                // 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);
+            }
+        );
         return;
     }
 
diff --git a/test/server.test.js b/test/server.test.js
index ba19cbb55..62c25927b 100644
--- a/test/server.test.js
+++ b/test/server.test.js
@@ -2051,6 +2051,51 @@ 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 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.equal(err.message, ERR_MSG);
+        if (gotReqCallback) {
+            t.end();
+        }
+    });
+
+    SERVER.on('uncaughtException', function(req, res, route, err, callback) {
+        callback();
+    });
+
+    SERVER.get('/foobar', function(req, res, next) {
+        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();
+        }
+    });
+});
+
 test('should increment/decrement inflight request count', function(t) {
     SERVER.get('/foo', function(req, res, next) {
         t.equal(SERVER.inflightRequests(), 1);