feat(server): remove aborted event handling, always listen for close

BREAKING CHANGE: server always throws RequestCloseError instead of RequestAbortedError

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.

lib/errorTypes.js

@@ -3,4 +3,3 @@
 var errors = require('restify-errors');
 
 errors.makeConstructor('RequestCloseError');
-errors.makeConstructor('RequestAbortedError');

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

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;
         }

lib/request.js

@@ -827,21 +827,20 @@ 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;
         return self._connectionState;
     };
 
     /**
-     * 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';
     };
 
     /**

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

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

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);