setKeepAlive silently ignores invalid values

[davepacheco]

While testing #21079, I also discovered that TCP sockets' setKeepAlive() function silently ignores invalid values. I tried passing 5000ms for the delay, which gets translated to 5s. On my system, the minimum is 10s, and the setsockopt returns with EINVAL, but Node neither throws an exception nor emits an Error on the socket. Here's the test program:

var sock = require('net').connect({
    'host': '172.25.10.4',
    'port': 12345
});
sock.on('connect', function () {
        console.log('setKeepAlive returns: ', sock.setKeepAlive(true, 5000));
});

and here's the output while tracing setsockopt():

$ truss -t setsockopt node tcpkatest1.js 
/1:     setsockopt(11, SOL_SOCKET, SO_KEEPALIVE, 0xFFFFFD7FFFDF8F70, 4, SOV_DEFAULT) = 0
/1:     setsockopt(11, tcp, 0x22, 0xFFFFFD7FFFDF8F7C, 4, SOV_DEFAULT) Err#22 EINVAL
setKeepAlive returns:  undefined

(On this system, 0x22 is TCP_KEEPIDLE. I checked that the value being passed in was "5", and that's less than the minimum allowed on this system.)

[bradisbell]

@davepacheco I think you may have solved a bug that I've been trying to track down for a long time. Could you share what platform you're on? I'm trying to figure out what the minimum keep alive time is for the systems I'm deploying to but am not finding much useful information.

[davepacheco]

I'm on illumos (SmartOS, specifically). There's some information on finding the value on various systems at http://www.gnugk.org/keepalive.html. Hope that helps!

[jameshartig]

It looks like libuv returns a negative number when setsockopt fails but the code in lib/net.js doesn't check the return type. The docs don't mention anything about a return value. I'm not sure if it should throw or return the error code from libuv. The problem with returning the error code from libuv is that 0 is success and negative number is fail, which would break:

if (!socket.setKeepAlive(true, 5000)) {
    console.log("it failed!");
}

Maybe it should just return a true/false?

[davepacheco]

I'd suggest treating this as a programming error and throwing an exception that should generally not be handled (except maybe to print a friendlier error message). We could also have emit an error on the socket so that programs could treat it as an operational error.

[misterdjules]

Emitting an 'error' event sounds good to me. Adding that to the 0.13.1 milestone as that would be a breaking change.

EDIT: Actually, throwing seems like it would make more sense, since most error return values from setsockopt indicate a programming error, not an operational error, and those who don't (ENOBUFS and ENOMEM) would probably not be recoverable.

[saquibkhan]

FYI...
Proper error handling for Socket.prototype._getsockname and Socket.prototype._getpeername is also not done and may need similar fix.

Socket.prototype._getpeername = function() {
  if (!this._peername) {
    if (!this._handle || !this._handle.getpeername) {
      return {};
    }
    var out = {};
    var err = this._handle.getpeername(out);
    if (err) return {};  // FIXME(bnoordhuis) Throw?
    this._peername = out;
  }
  return this._peername;
};

Socket.prototype._getsockname = function() {
  if (!this._handle || !this._handle.getsockname) {
    return {};
  }
  if (!this._sockname) {
    var out = {};
    var err = this._handle.getsockname(out);
    if (err) return {};  // FIXME(bnoordhuis) Throw?
    this._sockname = out;
  }
  return this._sockname;
};

[misterdjules]

@saquibkhan Good catch, thanks! 👍