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

net: give better error messages #7285

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
109 changes: 99 additions & 10 deletions lib/net.js
Original file line number Diff line number Diff line change
Expand Up @@ -742,7 +742,8 @@ function afterWrite(status, handle, req, err) {
}

if (status < 0) {
var ex = errnoException(status, 'write', err);
err = util.format('%s %s:%s', err || '', req.address, req.port);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious why the err || ''. If status < 0 then we should have an error object. Otherwise Node had a bug.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I feel like there was a previous test that was failing without that, but after looking like it, doesn't appear to be true.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's go ahead and remove it for now.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will do

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah found it. When passing an err to uv.errname, if the err if ENOTFOUND, I get this:

util.js:739
  var errname = uv.errname(err);
                   ^
Error: err >= 0
    at Error (native)
    at exports._errnoException (util.js:739:20)
    at net.js:941:18
    at Object.asyncCallback [as callback] (dns.js:78:16)
    at Object.onlookup [as oncomplete] (dns.js:91:17)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. Thanks for the info.

var ex = errnoException(status, 'write', err, req);
debug('write failure', ex);
self._destroy(ex, req.cb);
return;
Expand Down Expand Up @@ -804,28 +805,56 @@ function connect(self, address, port, addressType, localAddress, localPort) {
err = bind(localAddress, localPort);

if (err) {
self._destroy(errnoException(err, 'bind'));
var additions = {
address: address
};
var details;
if (port) {
details = util.format('%s:%s', address, port);
additions.port = port;
} else {
details = address;
}
var ex = errnoException(err, 'bind', details, additions);
self._destroy(ex);
return;
}
}

var req = { oncomplete: afterConnect };
var req = {
oncomplete: afterConnect,
port: undefined,
address: undefined,
localAddress: undefined,
localPort: undefined
};
if (addressType === 6 || addressType === 4) {
port = port | 0;
if (port <= 0 || port > 65535)
throw new RangeError('Port should be > 0 and < 65536');

req.port = port;
req.address = address;
if (addressType === 6) {
err = self._handle.connect6(req, address, port);
} else if (addressType === 4) {
err = self._handle.connect(req, address, port);
}
} else {
req.address = address;
err = self._handle.connect(req, address, afterConnect);
}

self._getsockname();
if (self._sockname) {
req.localAddress = self._sockname.address;
req.localPort = self._sockname.port;
}
if (err) {
self._destroy(errnoException(err, 'connect'));
var details = port ?
util.format('%s:%s', address, port) :
address;
self._destroy(errnoException(err, 'connect', details, req));
}
}

Expand Down Expand Up @@ -908,7 +937,15 @@ Socket.prototype.connect = function(options, cb) {
// There are no event listeners registered yet so defer the
// error event to the next tick.
process.nextTick(function() {
self.emit('error', err);
// The errno may not be recognized by libuv
var ex = util._extend(err, options);
var message = util.format('%s %s %s:%s',
err.syscall,
err.errno,
options.host,
options.port);
ex.message = message;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing the .message won't change the output of the stack. Is that what you were trying to do?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm, yea that doesn't make much sense. Should I just use errnoException here too?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll have to generate the error before the process.nextTick() or the stack trace will be lost. Possibly it needs to be changed to the following:

if (err) {
  var message = util.format('%s %s %s:%s',
                            err.syscall,
                            err.errno,
                            options.host,
                            options.port);
  var ex = new Error(message);
  util._extend(ex, options);
  // net.createConnection() creates a net.Socket object and
  // immediately calls net.Socket.connect() on it (that's us).
  // There are no event listeners registered yet so defer the
  // error event to the next tick.
  process.nextTick(function() {
    self.emit('error', ex);
    self._destroy();
  });
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, with this, we lose the original err properties though. Should we instead do something like

if (err) {
  var message = util.format('%s %s %s:%s',
                            err.syscall,
                            err.errno,
                            options.host,
                            options.port);
  var ex = errnoException(err, err.syscall, message, options);
  // net.createConnection() creates a net.Socket object and
  // immediately calls net.Socket.connect() on it (that's us).
  // There are no event listeners registered yet so defer the
  // error event to the next tick.
  process.nextTick(function() {
    self.emit('error', ex);
    self._destroy();
  });
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're correct. That's how it should be done. Nice catch.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like https://github.com/joyent/node/blob/master/lib/dns.js#L31-L52 explains the issue with ENOTFOUND.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems I've gotten a little lost on the actual problem. Though the issue was you wanted to expand the error message after the error has been created.

On the side, it's been discussed that err.errno should be set to the actual error instead of ENOTFOUND.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that a change that would need to be made in dns.js?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Here's an example:

diff --git a/lib/dns.js b/lib/dns.js
index 4eb34d6..4577397 100644
--- a/lib/dns.js
+++ b/lib/dns.js
@@ -29,12 +29,13 @@ var isIp = net.isIP;


 function errnoException(err, syscall, hostname) {
-  // FIXME(bnoordhuis) Remove this backwards compatibility shite and pass
-  // the true error to the user. ENOTFOUND is not even a proper POSIX error!
+  var errCode;
   if (err === uv.UV_EAI_MEMORY ||
       err === uv.UV_EAI_NODATA ||
       err === uv.UV_EAI_NONAME) {
-    err = 'ENOTFOUND';
+    errCode = 'ENOTFOUND';
+  } else {
+    errCode = err;
   }
   var ex = null;
   if (typeof err === 'string') {  // c-ares error code.

Doubt that is the fully correct solution, but I think that'll give you an idea.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So even with something like this, ENOTFOUND or EAI_NONAME could still throw in uv.errname(). So should we check this in util._errnoException?

self.emit('error', ex);
self._destroy();
});
} else {
Expand Down Expand Up @@ -975,7 +1012,17 @@ function afterConnect(status, handle, req, readable, writable) {

} else {
self._connecting = false;
self._destroy(errnoException(status, 'connect'));
var details = req.port ?
util.format('%s:%s', req.address, req.port) :
req.address;

if (req.localAddress && req.localPort) {
details = util.format('%s - Local (%s:%s)',
details,
req.localAddress,
req.localPort);
}
self._destroy(errnoException(status, 'connect', details, req));
}
}

Expand Down Expand Up @@ -1103,7 +1150,21 @@ Server.prototype._listen2 = function(address, port, addressType, backlog, fd) {
debug('_listen2: create a handle');
var rval = createServerHandle(address, port, addressType, fd);
if (util.isNumber(rval)) {
var error = errnoException(rval, 'listen');
var details, additions;
if (port > 0) {
details = util.format('%s:%s', address, port);
additions = {
address: address,
port: port
};
} else {
details = address;
additions = {
address: address

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may sound strange, but add port: null. The property will be skipped in errnoException() and by doing so the object map generated by V8 will remain the same. Do this for all similar locations.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should I go ahead and actually define additions before the conditional statement or just add port: null?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just add port: null. Object maps are generated by the names and order of object properties, so each object will generate the same object map.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, cool. Should have the changes up shortly. Probably should ask... do you want me to be squashing these commits?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't matter. If you don't then I will.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any progress?

};
}

var error = errnoException(rval, 'listen', details, additions);
process.nextTick(function() {
self.emit('error', error);
});
Expand All @@ -1120,7 +1181,20 @@ Server.prototype._listen2 = function(address, port, addressType, backlog, fd) {
var err = _listen(self._handle, backlog);

if (err) {
var ex = errnoException(err, 'listen');
var details, additions;
if (port > 0) {
details = util.format('%s:%s', address, port);
additions = {
address: address,
port: port
};
} else {
details = address;
additions = {
address: address
};
}
var ex = errnoException(err, 'listen', details, additions);
self._handle.close();
self._handle = null;
process.nextTick(function() {
Expand Down Expand Up @@ -1166,8 +1240,23 @@ function listen(self, address, port, addressType, backlog, fd) {
err = uv.UV_EADDRINUSE;
}

if (err)
return self.emit('error', errnoException(err, 'bind'));
if (err) {
var details, additions;
if (port > 0) {
details = util.format('%s:%s', address, port);
additions = {
address: address,
port: port
};
} else {
details = address;
additions = {
address: address
};
}
var ex = errnoException(err, 'bind', details, additions);
return self.emit('error', ex);
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drop the extra newline. We only have 2 newline characters between function declarations.

self._handle = handle;
self._listen2(address, port, addressType, backlog, fd);
Expand Down
12 changes: 11 additions & 1 deletion lib/util.js
Original file line number Diff line number Diff line change
Expand Up @@ -733,7 +733,7 @@ exports.pump = exports.deprecate(function(readStream, writeStream, callback) {


var uv;
exports._errnoException = function(err, syscall, original) {
exports._errnoException = function(err, syscall, original, additions) {
if (isUndefined(uv)) uv = process.binding('uv');
var errname = uv.errname(err);
var message = syscall + ' ' + errname;
Expand All @@ -743,5 +743,15 @@ exports._errnoException = function(err, syscall, original) {
e.code = errname;
e.errno = errname;
e.syscall = syscall;
if (additions && isObject(additions)) {
var keys = Object.keys(additions);
var len = keys.length;
for (var i = 0; i < len; i++) {
if (!isNullOrUndefined(additions[keys[i]]) &&
isPrimitive(additions[keys[i]])) {
e[keys[i]] = additions[keys[i]];
}
}
}
return e;
};
39 changes: 39 additions & 0 deletions test/simple/test-net-better-error-messages-listen-path.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
// Copyright Joyent, Inc. and other Node contributors.
//
// Permission is hereby granted, free of charge, to any person obtaining a
// copy of this software and associated documentation files (the
// "Software"), to deal in the Software without restriction, including
// without limitation the rights to use, copy, modify, merge, publish,
// distribute, sublicense, and/or sell copies of the Software, and to permit
// persons to whom the Software is furnished to do so, subject to the
// following conditions:
//
// The above copyright notice and this permission notice shall be included
// in all copies or substantial portions of the Software.
//
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN
// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
// USE OR OTHER DEALINGS IN THE SOFTWARE.

var common = require('../common');
var assert = require('assert');
var net = require('net');
var gotError = false;
var fp = '/blah/fadfa';
var server = net.createServer(function(socket) {
});
server.listen(fp, function() {
assert(false);
});
server.on('error', function(e) {
console.error('error', e);
gotError = true;
});

process.on('exit', function() {
assert(gotError);
});
41 changes: 41 additions & 0 deletions test/simple/test-net-better-error-messages-listen.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
// Copyright Joyent, Inc. and other Node contributors.
//
// Permission is hereby granted, free of charge, to any person obtaining a
// copy of this software and associated documentation files (the
// "Software"), to deal in the Software without restriction, including
// without limitation the rights to use, copy, modify, merge, publish,
// distribute, sublicense, and/or sell copies of the Software, and to permit
// persons to whom the Software is furnished to do so, subject to the
// following conditions:
//
// The above copyright notice and this permission notice shall be included
// in all copies or substantial portions of the Software.
//
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN
// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
// USE OR OTHER DEALINGS IN THE SOFTWARE.

var common = require('../common');
var assert = require('assert');
var net = require('net');
var gotError = false;

var server = net.createServer(function(socket) {
});
server.listen(1, '1.1.1.1', function() { // EACCESS or EADDRNOTAVAIL
assert(false);
});
server.on('error', function(e) {
console.error('error', e);
gotError = true;
assert.equal('1.1.1.1', e.address);
assert.equal(1, e.port);
});

process.on('exit', function() {
assert(gotError);
});
45 changes: 45 additions & 0 deletions test/simple/test-net-better-error-messages-path.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
// Copyright Joyent, Inc. and other Node contributors.
//
// Permission is hereby granted, free of charge, to any person obtaining a
// copy of this software and associated documentation files (the
// "Software"), to deal in the Software without restriction, including
// without limitation the rights to use, copy, modify, merge, publish,
// distribute, sublicense, and/or sell copies of the Software, and to permit
// persons to whom the Software is furnished to do so, subject to the
// following conditions:
//
// The above copyright notice and this permission notice shall be included
// in all copies or substantial portions of the Software.
//
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN
// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
// USE OR OTHER DEALINGS IN THE SOFTWARE.



var common = require('../common');
var net = require('net');
var assert = require('assert');
var fp = '/tmp/fadagagsdfgsdf';
var c = net.connect(fp);

c.on('connect', function() {
console.error('connected?!');
assert.ok(false);
});

var gotError = false;
c.on('error', function(e) {
console.error('couldn\'t connect.', e);
gotError = true;
assert.equal('ENOENT', e.code);
assert.equal(fp, e.address);
});

process.on('exit', function() {
assert.ok(gotError);
});
46 changes: 46 additions & 0 deletions test/simple/test-net-better-error-messages-port-hostname.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
// Copyright Joyent, Inc. and other Node contributors.
//
// Permission is hereby granted, free of charge, to any person obtaining a
// copy of this software and associated documentation files (the
// "Software"), to deal in the Software without restriction, including
// without limitation the rights to use, copy, modify, merge, publish,
// distribute, sublicense, and/or sell copies of the Software, and to permit
// persons to whom the Software is furnished to do so, subject to the
// following conditions:
//
// The above copyright notice and this permission notice shall be included
// in all copies or substantial portions of the Software.
//
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN
// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
// USE OR OTHER DEALINGS IN THE SOFTWARE.



var common = require('../common');
var net = require('net');
var assert = require('assert');

var c = net.createConnection(common.PORT, 'blah');

c.on('connect', function() {
console.error('connected?!');
assert.ok(false);
});

var gotError = false;
c.on('error', function(e) {
console.error('couldn\'t connect.', e);
gotError = true;
assert.equal('ENOTFOUND', e.code);
assert.equal(common.PORT, e.port);
assert.equal('blah', e.hostname);
});

process.on('exit', function() {
assert.ok(gotError);
});
Loading