refactor(overrider): cleanup socket event logic

Changes the logic around socket events in the Request Overrider, which
had comments that correctly denoted how confusing the code was compared
to what it was trying to achieve.

Previous logic monkey patched the `EventEmitter.on` method on the request
to watch for an addition of a `socket` listener and then emitted related
events. This was added explicitly to fix issue 79 where Nock would hang
when Restify was the client.
This change takes a different approach and attempts to mimic what a non-
mocked request would do.
`ClientRequest` always emits a `socket` event after a socket is assigned
to the request https://nodejs.org/api/http.html#http_event_socket
Then the socket emits connect and secureConnect events.

Note that the approach would more real to life, and cleaner, if the
socket itself was responsible for emitting the connect events. However,
doing so while ensuring they only fire after the Requests 'socket'
event requires at least two processes ticks and that breaks assumptions
in our `end` method. It sill might be a nice to have in the future, but
would require a larger refactor.

`once` on the request is no longer aliased to `on`.

Also moves yet another alias for a socket, `IncomingMessage.client`, up
to where the other aliases are defined.

lib/request_overrider.js

@@ -1,16 +1,17 @@
 'use strict'
 
+const debug = require('debug')('nock.request_overrider')
 const { EventEmitter } = require('events')
 const { IncomingMessage, ClientRequest } = require('http')
+const _ = require('lodash')
 const propagate = require('propagate')
-const DelayedBody = require('./delayed_body')
+const timers = require('timers')
+const zlib = require('zlib')
+
 const common = require('./common')
-const Socket = require('./socket')
-const _ = require('lodash')
-const debug = require('debug')('nock.request_overrider')
+const DelayedBody = require('./delayed_body')
 const globalEmitter = require('./global_emitter')
-const zlib = require('zlib')
-const timers = require('timers')
+const Socket = require('./socket')
 
 function getHeader(request, name) {
   return request.getHeader(name.toLowerCase())
@@ -99,11 +100,28 @@ function RequestOverrider(req, options, interceptors, remove) {
   // ClientRequest.connection is an alias for ClientRequest.socket
   // https://nodejs.org/api/http.html#http_request_socket
   // https://github.com/nodejs/node/blob/b0f75818f39ed4e6bd80eb7c4010c1daf5823ef7/lib/_http_client.js#L640-L641
-  // IncomingMessage.connection is an alias for IncomingMessage.socket
-  // https://github.com/nodejs/node/blob/b0f75818f39ed4e6bd80eb7c4010c1daf5823ef7/lib/_http_incoming.js#L44-L45
+  // IncomingMessage.connection & IncomingMessage.client are aliases for IncomingMessage.socket
+  // https://nodejs.org/api/http.html#http_response_socket
+  // https://github.com/nodejs/node/blob/b0f75818f39ed4e6bd80eb7c4010c1daf5823ef7/lib/_http_incoming.js#L44-L69
   // The same Socket is shared between the request and response to mimic native behavior.
-  req.socket = req.connection = response.socket = response.connection = Socket({
-    proto: options.proto,
+  req.socket = req.connection = new Socket({ proto: options.proto })
+  response.socket = response.client = response.connection = req.socket
+
+  propagate(['timeout'], req.socket, req)
+
+  // Emit a fake socket event on the next tick to mimic what would happen on a real request.
+  // Some clients listen for a 'socket' event to be emitted before calling end(),
+  // which causes nock to hang.
+  process.nextTick(() => {
+    req.emit('socket', req.socket)
+
+    // https://nodejs.org/api/net.html#net_event_connect
+    req.socket.emit('connect')
+
+    // https://nodejs.org/api/tls.html#tls_event_secureconnect
+    if (req.socket.authorized) {
+      req.socket.emit('secureConnect')
+    }
   })
 
   req.write = function(buffer, encoding, callback) {
@@ -186,28 +204,6 @@ function RequestOverrider(req, options, interceptors, remove) {
     emitError(connResetError)
   }
 
-  // restify listens for a 'socket' event to be emitted before calling end(),
-  // which causes nock to hang with restify. The following logic fakes the
-  // socket behavior for restify.
-  // Fixes https://github.com/nock/nock/issues/79
-  // TODO: This logic doesn't make total sense. It would be helpful to explain
-  // in a comment more fully what it is doing. Also it would be helpful to know
-  // if the problem in restify persists. In general we should do the standard
-  // thing, not implement workarounds for specific other modules.
-  // TODO: `req.once()` should not be an alias to `req.on()`. That is
-  // extraordinarily confusing behavior.
-  req.once = req.on = function(event, listener) {
-    // emit a fake socket.
-    if (event === 'socket') {
-      listener.call(req, req.socket)
-      req.socket.emit('connect', req.socket)
-      req.socket.emit('secureConnect', req.socket)
-    }
-
-    EventEmitter.prototype.on.call(this, event, listener)
-    return this
-  }
-
   const emitError = function(error) {
     process.nextTick(function() {
       req.emit('error', error)
@@ -474,18 +470,6 @@ function RequestOverrider(req, options, interceptors, remove) {
         return
       }
 
-      // `IncomingMessage.client` is an undocumented alias for
-      // `IncomingMessage.socket`. Assigning it here may help with
-      // compatibility, including with very old versions of `request` which
-      // inspect `response.client.authorized`. Modern versions of request
-      // inspect `response.socket.authorized` which is set to true in our
-      // `Socket` constructor.
-      // https://github.com/nock/nock/issues/158
-      // https://github.com/request/request/pull/1615
-      // https://nodejs.org/api/http.html#http_response_socket
-      // https://github.com/nodejs/node/blob/2e613a9c301165d121b19b86e382860323abc22f/lib/_http_incoming.js#L67
-      response.client = response.socket
-
       response.rawHeaders.push(
         ...selectDefaultHeaders(
           response.rawHeaders,

lib/socket.js

@@ -7,10 +7,6 @@ const util = require('util')
 module.exports = Socket
 
 function Socket(options) {
-  if (!(this instanceof Socket)) {
-    return new Socket(options)
-  }
-
   EventEmitter.apply(this)
 
   if (options.proto === 'https') {

tests/test_delay.js

@@ -28,7 +28,7 @@ function checkDuration(t, ms) {
       fin[0] * 1000 + // seconds -> ms
       fin[1] * 1e-6 // nanoseconds -> ms
 
-    /// innaccurate timers
+    /// inaccurate timers
     ms = ms * 0.9
 
     t.ok(

tests/test_request_overrider.js

@@ -11,6 +11,7 @@
 // the part of Nock that must interface with all http clients.
 
 const http = require('http')
+const https = require('https')
 const { URL } = require('url')
 const { test } = require('tap')
 const needle = require('needle')
@@ -431,6 +432,7 @@ test('socket is shared and aliased correctly', t => {
   req.once('response', res => {
     t.is(req.socket, req.connection)
     t.is(req.socket, res.socket)
+    t.is(res.socket, res.client)
     t.is(res.socket, res.connection)
     t.end()
   })
@@ -439,11 +441,11 @@ test('socket is shared and aliased correctly', t => {
 test('socket emits connect and secureConnect', t => {
   t.plan(3)
 
-  nock('http://example.test')
+  nock('https://example.test')
     .post('/')
     .reply(200, 'hey')
 
-  const req = http.request({
+  const req = https.request({
     host: 'example.test',
     path: '/',
     method: 'POST',