Add option for connect timeout

[dougwilson]

This adds a new option connectTimeout which specifies a timeout (in milliseconds) for the initial connection to timeout. This applies to the time between sending data to the remote server and establishing a TCP connection. The default value is to not have a timeout, which makes it backwards-compatible.

This timeout is named connectTimeout because it specifically ONLY applies to the connection establishment, nothing else. This means that the timeout will no longer time out any operation after the connection has been successfully established.

Fixes #708
Fixes #717

[sidorares]

lgtm 👍

[dresende]

👍

[dougwilson]

Hey guys, I do need to change one thing on this PR, though. Right now a timeout event will get emitted and sent down to a protocol error, but the socket itself will stay there still working on getting connected; I need to change it to destroy the socket when the timeout occurs still.

[sidorares]

why do you need to pass handler as argument here?

[dougwilson]

This is needed to remove the specific timeout listener. this.setTimeout(0, handleConnectTimeout); is the same as this.setTimeout(0); this.removeListener('timeout', handleConnectTimeout). The timeout listener needs to be removed so it does not trigger after we have established a connection, since this is just a timeout for connecting.

[dougwilson]

This if block could also be written as:

if (this.config.connectTimeout) {
  var handleConnectTimeout = this._handleConnectTimeout.bind(this);

  this._socket.setTimeout(this.config.connectTimeout);
  this._socket.once('timeout', handleConnectTimeout);
  this._socket.once('connect', function() {
    this.setTimeout(0);
    this.removeListener('timeout', handleConnectTimeout);
  });
}

if that makes it clearer.

[sidorares]

I just checked setTimeout(0) without handler and previous timer is not called

var net = require('net');
var s = net.connect(80, 'stackoverflow.com');
s.setTimeout(1000, function() { console.log('timeout');  });
s.on('connect', function() {
  console.log('connected!');
  s.setTimeout(0);
});

[dougwilson]

Correct, because when you run a.setTimeout(0) the socket will never emit a timeout event any more, because the timeout timers are unenrolled. But if some user-land code is setting a timeout on our socket, our timeout listener will then suddenly trigger. We need to remove our listener to be safe. Try:

var net = require('net');
var s = net.connect(80, 'stackoverflow.com');
s.setTimeout(1000, function() { console.log('timeout');  });
s.on('connect', function() {
  console.log('connected!');
  s.setTimeout(0);
  setTimeout(function(){
    // Sometime later some user code does
    s.setTimeout(2);
    // Which didn't do anything before, or maybe they are adding their own
    // timeout listener for some reason
    // Our timeout listener is now triggered since we never removed it
  },100);
});

[dougwilson]

I am just trying to make sure that this timeout function is only triggered for a timeout during the connection establishment and not at any possible later point, which is why I went through extra effort to remove the timeout listener we added.

[sidorares]

Good point

[sidorares]

@dougwilson could you add a line to changelog as well please?

[dougwilson]

@sidorares done :)

[sidorares]

Interesting, domains test fail on node 0.8

 AssertionError: "TypeError: Cannot call method 'release' of undefined" == "Error: inside domain 3"
      at Domain.d1.run.d4.on.err4 (/home/travis/build/felixge/node-mysql/test/integration/connection/test-domains.js:68:12)
      at Domain.EventEmitter.emit (events.js:96:17)
      at process.uncaughtHandler (domain.js:61:20)
      at process.EventEmitter.emit (events.js:126:20)

[dougwilson]

I re-ran the tests on Travis and it passed now. That test is flawed, because it does not check err from pool.getConnection in d3, so it is hard to know what the real error was. It was likely some error from the Travis CI MySQL, was was transient, as they passed this time without changing anything.

[sidorares]

Great, merging

[dougwilson]

@sidorares I also just sent PR #728 to re-throw those errors so if an error occurs at points in the domains test, we can see what happened on Travis CI.

[felixge]

💖 thx for all the PRs @dougwilson !

[felixge]

@dougwilson also just gave you commit bits / npm push access. Enjoy!

[dougwilson]

Thanks, @felixge It shall not be abused!