chore: opinionated clean up of once callbacks (#180)

lib/HttpClient.js

@@ -96,7 +96,7 @@ function rawRequest(opts, cb) {
     assert.func(cb, 'callback');
 
     /* eslint-disable no-param-reassign */
-    cb = once(cb);
+    cb = once.strict(cb);
     /* eslint-enable no-param-reassign */
 
     var id = dtrace.nextId();
@@ -114,15 +114,10 @@ function rawRequest(opts, cb) {
     var connectionTimer;
     var requestTimer;
     var req;
-
-    // NOTE:
-    // both the following events, `req.on('result'...)` and `client.on('after',
-    // ...)` are currently 'once'd but it really shouldn't need to be. this is
-    // required because a req.on('error') event is triggered when
-    // client.close() is called due to the way we forcibly tear down sockets.
-    // inside the 'error' listener, we emit the result event with an error,
-    // even though it may have emitted a result previously. with a once.strict()
-    // this throws - definitely worth revisiting at some point.
+    // flag that captures whether or not user provided callback has been
+    // invoked yet. this is an alternative to using once() that is more
+    // explicit.
+    var cbCalled = false;
 
     /**
      * this function is called after the request lifecycle has been "completed"
@@ -136,7 +131,7 @@ function rawRequest(opts, cb) {
      * @param {Object} [_res] response object
      * @return {undefined}
      */
-    var emitAfter = once(function _emitAfter(_req, _res, _err) {
+    var emitAfter = once.strict(function _emitAfter(_req, _res, _err) {
         assert.optionalObject(_err, '_err');
         assert.object(_req, '_req');
         assert.optionalObject(_res, '_res');
@@ -166,7 +161,7 @@ function rawRequest(opts, cb) {
      * @param {Object} [_res] response object
      * @return {undefined}
      */
-    var emitResult = once(function _emitResult(_err, _req, _res) {
+    var emitResult = once.strict(function _emitResult(_err, _req, _res) {
         assert.optionalObject(_err, '_err');
         assert.object(_req, '_req');
         assert.optionalObject(_res, '_res');
@@ -215,17 +210,8 @@ function rawRequest(opts, cb) {
             // this first before we abort the request so we can pick up socket
             // information like the ip.
             var err = errors.createConnectTimeoutErr(opts, req);
-
-            if (req) {
-                req.abort();
-            }
-
-            dtrace._rstfy_probes['client-error'].fire(function () {
-                return ([id, err.toString()]);
-            });
-            cb(err, req);
-            emitResult(err, req, null);
-            emitAfter(req, null, err);
+            req._forcedAbortErr = err;
+            req.abort();
         }, opts.connectTimeout);
     }
 
@@ -301,41 +287,45 @@ function rawRequest(opts, cb) {
     req.log = log;
 
     var startRequestTimeout = function startRequestTimeout() {
+        // the request object must already exist before we can set a timeout
+        // on it.
+        assert.object(req, 'req');
+
         if (opts.requestTimeout) {
             requestTimer = setTimeout(function requestTimeout() {
                 requestTimer = null;
 
                 var err = errors.createRequestTimeoutErr(opts, req);
-                dtrace._rstfy_probes['client-error'].fire(function () {
-                    return ([id, err.toString()]);
-                });
-
-                cb(err, req);
-
-                if (req) {
-                    req.abort();
-                }
-                process.nextTick(function () {
-                    emitResult(err, req, null);
-                    emitAfter(req, null, err);
-                });
+                req._forcedAbortErr = err;
+                req.abort();
             }, opts.requestTimeout);
         }
     };
 
     req.on('error', function onError(err) {
+        var realErr = req._forcedAbortErr || err;
         dtrace._rstfy_probes['client-error'].fire(function () {
-            return ([id, (err || {}).toString()]);
+            return ([id, (realErr || {}).toString()]);
         });
-        log.trace({err: err}, 'Request failed');
+        log.trace({ err: realErr }, 'Request failed');
         clearTimeout(connectionTimer);
         clearTimeout(requestTimer);
 
-        cb(err, req);
+        // the user provided callback is invoked as soon as a connection is
+        // established. however, the request can be aborted or can fail after
+        // this point. any errors that occur after connection establishment are
+        // not propagated via the callback, but instead propagated via the
+        // 'result' event. check the cbCalled flag to ensure we don't double
+        // call the callback. while this could be handled by once, this is more
+        // explicit and easier to reason about.
+        if (cbCalled === false) {
+            cb(realErr, req);
+            cbCalled = true;
+        }
 
         process.nextTick(function () {
-            emitResult(err, req, null);
-            emitAfter(req, null, err);
+            emitResult(realErr, req, null);
+            emitAfter(req, null, realErr);
         });
     });
 
@@ -407,7 +397,13 @@ function rawRequest(opts, cb) {
                 socket.setKeepAlive(true);
             }
 
+            // eagerly call the user provided callback here with the request
+            // obj after we've established a connection. note that it's still
+            // possible for the request to timeout at this point, but a
+            // RequestTimeoutError would be triggered through the 'result' event
+            // and not the callback.
             cb(null, req);
+            cbCalled = true;
         });
     });
 
@@ -793,7 +789,7 @@ HttpClient.prototype.request = function request(opts, cb) {
     assert.func(cb, 'callback');
 
     /* eslint-disable no-param-reassign */
-    cb = once(cb);
+    cb = once.strict(cb);
     /* eslint-enable no-param-reassign */
 
     opts.audit = this.audit;