Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

net.Socket has no setConnectionTimeout(timeout) method #5757

Closed
DanielYWoo opened this issue Mar 17, 2016 · 16 comments
Closed

net.Socket has no setConnectionTimeout(timeout) method #5757

DanielYWoo opened this issue Mar 17, 2016 · 16 comments
Labels
feature request Issues that request new features to be added to Node.js. net Issues and PRs related to the net subsystem.

Comments

@DanielYWoo
Copy link

  • Version: v5.7.1
  • Platform: Mac OSX Yosemite 64bit
  • Subsystem: net

Currently we can only set timeout for an established idle socket, I cannot find a way to set connection timeout. This is missing piece, right?

@mscdex mscdex added net Issues and PRs related to the net subsystem. feature request Issues that request new features to be added to Node.js. labels Mar 17, 2016
@alvieirajr
Copy link

The socket.setTimeout(timeout[, callback]) doesn't works to you ? (Node.js - Net Module)

@DanielYWoo
Copy link
Author

setTimeout is for socket timeout which means the max idle time (no bytes received or sent) for an established connection, what I need is a connection timeout, that's the max time to establish a connection.

@piiiet
Copy link

piiiet commented Jul 27, 2016

Just need the same.

@tniessen
Copy link
Member

tniessen commented Jun 3, 2017

I don't think libuv supports this. Usually, people handle connect timeouts by putting the socket into non-blocking mode before connecting and then waiting for select to either run into a timeout or a status change on the socket. In node, you don't have the problem of blocking sockets, so you can just define your own timeout using setTimeout() and socket.destroy().

@bnoordhuis
Copy link
Member

socket.setTimeout() works for this: it sets a timer that expires after n milliseconds of inactivity; that includes connecting. Example:

var socket = net.connect(/* ... */);
socket.setTimeout(5e3, () => socket.destroy());
socket.once('connect', () => socket.setTimeout(0));

I'm closing this as the existing APIs have this covered.

@falahati
Copy link

falahati commented Nov 5, 2019

The solution provided by @bnoordhuis is unreliable and might not work. I had no success getting it to work on Windows 10 (Electron 7). The reality of the situation is that setTimeout() only changes the SO_RCVTIMEO and the SO_SNDTIMEO options and since they are not meant to be used for connecting; their effect generally depends on the environment or the underlying implementation and therefore should not be relied upon.
And since there is no way to set other socket options manually (otherwise TCP_USER_TIMEOUT could have been an option) the best way is to use
global.setTimeout() to destroy the socket to force the connection to be dropped. This is essentially what @tniessen suggested before.

However, it would be great if we could have used the setsockopt() method directly.

@DanielYWoo
Copy link
Author

DanielYWoo commented Nov 6, 2019

@falahati Does this work?

var socket = net.connect(/* ... */);
var t = setTimeout(CONN_TIMEOUT, () => socket.destroy());
socket.once('connect', () => {clearTimeout(t); socket.setTimeout(SOCK_TIMEOUT);});

@falahati
Copy link

falahati commented Nov 6, 2019

Essentially yeah. My code is a little more complicated but in the end the same underlying logic is at work.

isAlive(): Promise<boolean> {
    if (!this.config || !this.config.portNumber || !this.config.ipAddress) {
        return Promise.resolve(false);
    }

    var client = new net.Socket();

    function destroy() {
        if (client != null) {
            client.destroy();
            client = null;
        }
    }

    return new Promise<boolean>(
            (resolve) => {
                try {
                    client.once(
                        "connect",
                        () => {
                            destroy();
                            resolve(true);
                        }
                    );

                    client.once(
                        "error",
                        () => {
                            destroy();
                            resolve(false);
                        }
                    );

                    client.connect(this.config.portNumber, this.config.ipAddress);
                } catch (e) {
                    destroy();
                    resolve(false);
                }
            }
        )
        .timeout(2000)
        .catch(() => {
                destroy();
                return false;
            }
        );
}

With timeout() coming from here:

Promise.delay = (duration: number) => new Promise((resolve) => setTimeout(() => resolve(), duration));

Promise.prototype.timeout = function (duration): Promise<any> {
    return new Promise((resolve, reject) => {
        this.then(value => { resolve(value) });
        Promise.delay(duration).then(() => { reject(new Error("timed out")) });
    });
};

@kvelaro
Copy link

kvelaro commented Feb 23, 2020

Have exactly the same problem, and actually found good solution

let client = net.connect({...})

this setTimeout works only for sockets whose connection established

client.setTimeout(10000, (e) => {});
client.on('data', function (data) {});
client.on('error', (e) => {});
client.on('end', function () {});

general timeout, if we dont want to wait time set by default,
then we should emit error ourselfs

 timer = setTimeout(function() {
            client.emit('error', new Error('Connection could not be established'))
  }, 15000);

You are welcome :)

@DanielYWoo
Copy link
Author

@kvelaro the timeout you mentioned is for the whole duration including establishing the connection, transferring, and finally closing. What we need here is the just connection timeout.

@gbidkar
Copy link

gbidkar commented Apr 16, 2020

Can we re-open this? The solutions given by @kvelaro and @falahati do work but it does make sense if this will be included in the API.

@Antonius-S
Copy link

Antonius-S commented May 14, 2020

@gbidkar actually the existing API works. Tested with Node 10.17 and 13.2

'use strict';

const net = require('net');
const events = require('events');

const sock = new net.Socket();
sock.setTimeout(5000);
sock.on('timeout', () => sock.emit('error', new Error('ETIMEDOUT')));
sock.on('error', (e) => console.log((new Date()).toISOString(), 'Error', e.message));
sock.connect(111, '8.8.8.8');
console.log((new Date()).toISOString(), 'Connecting');

Prints

2020-05-14T08:39:09.882Z Connecting
2020-05-14T08:39:14.883Z Error ETIMEDOUT
2020-05-14T08:39:30.890Z Error connect ETIMEDOUT 8.8.8.8:111

Note that by default socket already has 20 sec timeout that won't be overridden by your custom one (the last log line) even if timeout is set to 0. So unless the socket is destroyed error will be thrown by socket's internals.
This also means you must handle this error specially if you want greater connect timeout.

@DanielYWoo
Copy link
Author

@Antonius-S The problem is to set connection timeout, not the socket timeout.

@Antonius-S
Copy link

@DanielYWoo IDK what do you mean by 'connection' and 'socket' timeout but my code detects timeout during connect.

@falahati
Copy link

@Antonius-S
I described here why this solution is unreliable. Please read the older comments.

@gbidkar
Unfortunately, I don't believe that the Net library should do anything more than what it does now directly regarding the timeout. I believe it makes it more complicated for people who have used sockets in almost every other language.
The problem described in this issue is present when using C++, C#, Python, and others. This is due to the fact that the Net library and other similar counterparts are simply wrappers around the OS's socket API.
In fact, I believe this issue should be limited to collaborators.

However, I strongly ask for a direct setSocketOption() method so that the programmer can directly communicate with the underlying socket implementation if required. But that a separate issue.

@Antonius-S
Copy link

@falahati

I described here why this solution is unreliable. Please read the older comments.

I've read your comment before posting. Honestly I'm neither an expert in Node internals nor have an idea what your opinion is based on, I just prefer to trust the facts.
Fact №1. Sources of Socket.prototype.setTimeout clearly say that instance of Timeout is created and bound to socket.
Fact №2. My code above runs as expected on Win7, Win10 and even Linux (I tested it on two online Node playgrounds which both run Linux).

Your move ;-P

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. net Issues and PRs related to the net subsystem.
Projects
None yet
Development

No branches or pull requests

10 participants