fix: handle connection after connect event was emitted (#1095)

It's possible for connect promise to resolve after the resolved
connection has already been established, which means we miss the connect
event. This can happen when Bluebird is set as the global Promise, and
we connect to an ip address host for example. This fix checks to see if
that's the case and invokes the connection handler directly. Thanks
@luin for the recommendation.

Fixes #977

Author: alavers

Diff for: lib/redis/index.ts

@@ -304,41 +304,45 @@ Redis.prototype.connect = function (callback) {
           stream.setKeepAlive(true, options.keepAlive);
         }
 
-        stream.once(CONNECT_EVENT, eventHandler.connectHandler(_this));
+        if (stream.connecting) {
+          stream.once(CONNECT_EVENT, eventHandler.connectHandler(_this));
+
+          if (options.connectTimeout) {
+            /*
+             * Typically, Socket#setTimeout(0) will clear the timer
+             * set before. However, in some platforms (Electron 3.x~4.x),
+             * the timer will not be cleared. So we introduce a variable here.
+             *
+             * See https://github.com/electron/electron/issues/14915
+             */
+            let connectTimeoutCleared = false;
+            stream.setTimeout(options.connectTimeout, function () {
+              if (connectTimeoutCleared) {
+                return;
+              }
+              stream.setTimeout(0);
+              stream.destroy();
+
+              const err = new Error("connect ETIMEDOUT");
+              // @ts-ignore
+              err.errorno = "ETIMEDOUT";
+              // @ts-ignore
+              err.code = "ETIMEDOUT";
+              // @ts-ignore
+              err.syscall = "connect";
+              eventHandler.errorHandler(_this)(err);
+            });
+            stream.once(CONNECT_EVENT, function () {
+              connectTimeoutCleared = true;
+              stream.setTimeout(0);
+            });
+          }
+        } else {
+          process.nextTick(eventHandler.connectHandler(_this));
+        }
         stream.once("error", eventHandler.errorHandler(_this));
         stream.once("close", eventHandler.closeHandler(_this));
 
-        if (options.connectTimeout) {
-          /*
-           * Typically, Socket#setTimeout(0) will clear the timer
-           * set before. However, in some platforms (Electron 3.x~4.x),
-           * the timer will not be cleared. So we introduce a variable here.
-           *
-           * See https://github.com/electron/electron/issues/14915
-           */
-          let connectTimeoutCleared = false;
-          stream.setTimeout(options.connectTimeout, function () {
-            if (connectTimeoutCleared) {
-              return;
-            }
-            stream.setTimeout(0);
-            stream.destroy();
-
-            const err = new Error("connect ETIMEDOUT");
-            // @ts-ignore
-            err.errorno = "ETIMEDOUT";
-            // @ts-ignore
-            err.code = "ETIMEDOUT";
-            // @ts-ignore
-            err.syscall = "connect";
-            eventHandler.errorHandler(_this)(err);
-          });
-          stream.once(CONNECT_EVENT, function () {
-            connectTimeoutCleared = true;
-            stream.setTimeout(0);
-          });
-        }
-
         if (options.noDelay) {
           stream.setNoDelay(true);
         }

Diff for: test/functional/connection.ts

@@ -4,6 +4,7 @@ import * as sinon from "sinon";
 import { expect } from "chai";
 import MockServer from "../helpers/mock_server";
 import * as Bluebird from "bluebird";
+import { StandaloneConnector } from "../../lib/connectors";
 
 describe("connection", function () {
   it('should emit "connect" when connected', function (done) {
@@ -423,5 +424,31 @@ describe("connection", function () {
         done();
       });
     });
+
+    it("works when connection established before promise is resolved", (done) => {
+      const socket = new net.Socket();
+      sinon.stub(StandaloneConnector.prototype, "connect").resolves(socket);
+      socket.connect(6379, "127.0.0.1").on("connect", () => {
+        new Redis().on("connect", () => done());
+      });
+    });
+
+    it("ignores connectTimeout when connection established before promise is resolved", (done) => {
+      const socketSetTimeoutSpy = sinon.spy(net.Socket.prototype, "setTimeout");
+      const socket = new net.Socket();
+      sinon.stub(StandaloneConnector.prototype, "connect").resolves(socket);
+      socket.connect(6379, "127.0.0.1").on("connect", () => {
+        const redis = new Redis({
+          connectTimeout: 1,
+        });
+        redis.on("error", () =>
+          done(new Error("Connect timeout should not have been called"))
+        );
+        redis.on("connect", () => {
+          expect(socketSetTimeoutSpy.callCount).to.eql(0);
+          done();
+        });
+      });
+    });
   });
 });