Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

socket.setTimeout malfunction when timeout is string #8618

Closed
weihua44 opened this issue Oct 25, 2014 · 6 comments
Closed

socket.setTimeout malfunction when timeout is string #8618

weihua44 opened this issue Oct 25, 2014 · 6 comments
Labels

Comments

@weihua44
Copy link

Test the following code,
node tst

telnet localhost 10015, the connection will be lost in 1 second
telnet locahost 10016 then telnet localhost 10015, the connection will not be lost (it means the timeout event is failed for some reason)
If you change the var "to" from string to int, the timeout will always working.

<<File: tst.js>>

var net = require('net');

var clilistener = net.createServer(function(c) {
    var to  = '1000';
//  var to  = 1000;
    var mark    = 'svr1:';
    console.log(mark+'cli connected from '+c.remoteAddress+':'+c.remotePort);
    console.log(mark+'set timeout to '+to);
    c.setTimeout(to, function() {
        console.log(mark+'time out arrived...');
        c.destroy();
    });
    c.on('error', function(err) {
        c.destroy();
    });
    c.on('close', function(isErr) {
        console.log(mark+'cli out');
    });
    //
});
clilistener.listen(10015);

var clilistener = net.createServer(function(c) {
    var to  = '60000';
//  var to  = 60000;
    var mark    = 'svr1:';
    console.log(mark+'cli connected from '+c.remoteAddress+':'+c.remotePort);
    console.log(mark+'set timeout to '+to);
    c.setTimeout(to, function() {
        console.log(mark+'time out arrived...');
        c.destroy();
    });
    c.on('error', function(err) {
        c.destroy();
    });
    c.on('close', function(isErr) {
        console.log(mark+'cli out');
    });
    //
});
clilistener.listen(10016);
@cjihrig
Copy link

cjihrig commented Oct 26, 2014

@weihua44 are you able to try this commit - cjihrig/node@7f3261c? It seems to fix the problem on my end.

@weihua44
Copy link
Author

@cjihrig Thanks, it works. And you taught me another way to change string to number ;-)

@trevnorris
Copy link

@cjihrig Is this something that should be merged?

@trevnorris trevnorris added the net label Dec 30, 2014
@cjihrig
Copy link

cjihrig commented Dec 30, 2014

@trevnorris there was some back and forth on the best way to fix bad timeout values. The most current fix for this is #8884

@trevnorris
Copy link

@cjihrig Thanks. Commented on that.

cjihrig added a commit that referenced this issue Jan 22, 2015
This commit restricts socket timeouts non-negative, finite
numbers. Any other value throws a TypeError or RangeError.
This prevents subtle bugs that can happen due to type
coercion.

Fixes: #8618
PR-URL: #8884
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: Timothy J Fontaine <[email protected]>
@cjihrig
Copy link

cjihrig commented Jan 22, 2015

Closed in f347573

@cjihrig cjihrig closed this as completed Jan 22, 2015
cjihrig added a commit to nodejs/node that referenced this issue Feb 13, 2015
This commit restricts socket timeouts non-negative, finite
numbers. Any other value throws a TypeError or RangeError.
This prevents subtle bugs that can happen due to type
coercion.

Fixes: nodejs/node-v0.x-archive#8618
PR-URL: nodejs/node-v0.x-archive#8884
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: Timothy J Fontaine <[email protected]>

Conflicts:
	lib/timers.js
	test/simple/test-net-settimeout.js
	test/simple/test-net-socket-timeout.js
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

3 participants