Allow specifying connection timeout in socket options

[rexxars]

Since connect() does not return the socket instance, there is currently no easy way to set the connection timeout. This PR adds a timeout option to the socket options passed to connect(), and automatically sets it on the socket when connecting. If a timeout is reached, it will trigger the connect callback with an error.

This should solve #88 .

[jcrugzz]

I like this idea @rexxars but the timeout should be the default and not 0 if a timeout option is not specified. So i'd wrap that in an if clause instead.

[rexxars]

I agree there should be a default timeout - however, the default as of now is no timeout. Introducing a default timeout might be considered to be a breaking change, so I'd want to OK that with the maintainer first.

Like I said, the default is 0, meaning no timeout. I could use an if clause, but in theory it should do pretty much the exact same as it does now.

[jcrugzz]

@rexxars yea thats totally my fault. I was assuming HTTP here which does have a default timeout. My apologies, this does look good.

[squaremo]

I could use an if clause, but in theory it should do pretty much the exact same as it does now.

Supplying 0 to Socket#setTimeout means "clear any timeout", which is not what's intended. (http://nodejs.org/api/net.html#net_socket_settimeout_timeout_callback)
It is harmless as things stand, since the socket is created then and there, but it might be a minor timebomb.

[rexxars]

I didn't bother doing it since the socket was created in place, but optimizing for change makes sense. Latest commit checks if the timeout specified in options is actually set before assigning the timeout handler.

[squaremo]

Thanks @rexxars. I tweaked it slightly -- are you happy with 7ccd837 ?

[rexxars]

Yup, looks good!

[zweifisch]

Seems that this does not affect the timeout value for net.connect(which is about 2 minutes on my machine), since sock.setTimeout is put inside the onConnect function. Is it intended?

[squaremo]

Seems that this does not affect the timeout value for net.connect(which is about 2 minutes on my machine), since sock.setTimeout is put inside the onConnect function. Is it intended?

That .. is a good point @zweifisch. Casting my mind back, I think perhaps it was only intended to cover a failure to complete the handshake. But failing to establish a connection seems like something it could also cover with the minor modification you imply.

What do you think, @rexxars?

[rexxars]

I agree, this looks like an oversight. I think this fixed a specific problem I had, but moving it outside the onConnect method should cover both connection and handshake timeouts.