Socket connection timeouts don't fire

[damons-sportsbet]

• I've searched for any related issues and avoided creating a duplicate
  issue.

Description

Setting a handshake timeout for a connection doesn't work if the socket never connects.

I want my app to fail fast if it can't connect to an endpoint, but I found that if I set a handshakeTimeout of 5 seconds and configured my app to talk to a known machine but on a port that just silently drops connecting packets, then it didn't time out in 5 seconds but instead would get a global ETIMEDOUT error.

Looking at the code for websocket.js it does this:

var req = (this._req = httpObj.get(options));

  if (options.handshakeTimeout) {
    req.on('timeout', () => {
      abortHandshake(this, req, 'Opening handshake has timed out');
    });
}

but I note that the advice for this is to wait for a socket to be assigned before setting a timeout handler. Testing this myself, I also found that just waiting for a socket wasn't enough to get the https library to handle the timeout properly, I had to also set an empty socket timeout handler on the socket itself.

So if I rewrite the code above as:

	this._req = httpObj.get(requestOptions)

	this._req.on("socket", socket => {
		if (options.handshakeTimeout) {
			this._req.setTimeout(options.handshakeTimeout, () => {
				this._req.abort()
				this.finalize(new Error("opening handshake has timed out"))
			})
			socket.setTimeout(options.handshakeTimeout, () => {
				// no-op - just causes above request setTimeout to work
			})
		}
	})

(bearing in mind you appear to have made the req var local between my version and HEAD) then I get successful 5 second timeout errors from websockets.

If you like I can make a PR for the change, I just want to make sure it's not some platform dependent issue or other strange workaround.

Thanks!

Reproducible in:

• version: 6.1.3
• Node.js version(s): 10.9.0
• OS version(s): MacOS 10.13.6

Steps to reproduce:

1. Create a Websockets instance connecting to a known machine and port that drops packets

2. Set a handshakeTimeout of 5 seconds

3. Set a timeout handler and see that it does not time out in 5 seconds but instead waits the minute or two for the global error handler to fire.

Expected result:

Timeout handler to fire

Actual result:

no timeout fired

Attachments:

[lpinca]

It was like this because it was using request.setTimeout(), that method set the timeout on the socket after the socket connects, but it was changed in 63adb73. Now the timeout is set when the socket is assigned to the request.

I cannot reproduce with the steps you listed above. Here is my test case:

'use strict';

const WebSocket = require('ws');

const start = Date.now();

// Use a non-routable IP address so the connection is never established.
const ws = new WebSocket('ws://192.0.2.1', {
  handshakeTimeout: 5000
});

ws.on('error', function(error) {
  const elapsed = Date.now() - start;
  console.log('Socket closed after %dms', elapsed);
  console.error(error);
});

which prints

Socket closed after 5012ms
Error: Opening handshake has timed out
    at ClientRequest.req.on (/Users/luigi/gh-1523/node_modules/ws/lib/websocket.js:545:7)
    at ClientRequest.emit (events.js:197:13)
    at Socket.emitRequestTimeout (_http_client.js:669:40)
    at Object.onceWrapper (events.js:285:13)
    at Socket.emit (events.js:197:13)
    at Socket._onTimeout (net.js:447:8)
    at listOnTimeout (timers.js:327:15)
    at processTimers (timers.js:271:5)

The issue with tls has been fixed in nodejs/node#25517, released with Node.js 11.8.0 (hopefully it will be backported).

[larvata]

After I upgraded the ws to 6.1.4, it works as expected.
Tested with node 10.11.0.

[lpinca]

I'm closing this, discussion can continue if needed.

[andrewlorenz]

Timeout not functioning for ws 6.2.1 on node v8.11.4. No mention in the thread that node upgrade was requirement to fix ?