Add socket_connect_timeout option.

[benjie]

This allows you to specify the connection timeout for the stream; note that once the stream is connected the existing code performs stream.setTimeout(0) thereby preventing this patch triggering an idle timeout on an established connection. (This is also why I called it socket_connect_timeout rather than simply socket_timeout.)

I added this.stream.setTimeout to install_stream_listeners as it seemed the DRY-est place to put it since it is called for all streams just after they're created. If this is not the right place, please let me know.

[BridgeAR]

@bengl LGTM would you please rebase and add some tests?

[bengl]

by @bengl you mean @benjie right? 😄

[BridgeAR]

@bengl ups sorry for bringing you up :D
And yes, I meant @benjie

[benjie]

😆 thanks @bengl :)

@BridgeAR I'm not sure how I'd write a test for this

[benjie]

Rebased :)

[BridgeAR]

Please use

debug("Stream timeout " + this.address + " id " + this.connection_id);

instead

[BridgeAR]

@benjie shouldn't it be enough if you just manipulate the socket timeout with the parameter? What you want to reach is to have a individual timeout instead of the system one, so everything else should stay the same?

And a test could be written by checking the system timeout (use a before, create a client that can't connect and check for the timeout and safe that value). Now you can increase or decrease that value as you want and do the same again and check that the timeout triggered at another time.

[benjie]

What endpoint should I use that it times out connecting to? I think a closed port won't do because it'll just drop the connection immediately (no chance for a timeout) so it'd need to be one that drops the packets. Adding an iptables filter would make the test platform dependent; is there a cross-platform /dev/null equivalent for TCP?

[benjie]

(debug() change implemented)

[BridgeAR]

You could mock the stream :)

Google just gave me this: https://gist.github.com/mjackson/1201196

[BridgeAR]

@benjie I used your work to use the connection_timeout as socket timeout if explicitly provided and mentioned you in the changelog. That way we do not have to add another option and most people thought the connection timeout would also set a socket timeout.

Thx a lot for your help!

[benjie]

Thanks @BridgeAR; sorry I didn't get around to writing the tests yet but your solution's probably better anyway! 😄

[benjie]

Hey @BridgeAR; I think your fix serves a different purpose to my PR - I want the connection attempt to fail, be closed, and then attempt again in a much tighter loop than the OS default (like every 30 seconds rather than 180); whereas connect_timeout stops attempting to reconnect meaning we remain disconnected forever. There may still be value in this PR

[billinghamj]

This should really be reopened

[BridgeAR]

@benjie thanks for pointing this out and sorry for not understanding your original issue properly right away.

There seems to be a new PR to do the same: #1430

[benjie]

No problem :)