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: add localAddress/Port for better error msgs #3946

Closed
wants to merge 4 commits into from

Conversation

Projects
None yet
6 participants
@jscissr
Copy link
Contributor

commented Nov 20, 2015

That could result in an error being thrown while creating an error...

@mscdex mscdex added the net label Nov 20, 2015

@jasnell

This comment has been minimized.

Copy link
Member

commented Nov 20, 2015

how odd that this wasn't caught before. LGTM but it would be helpful to have a quick test case included that fails before this is fix but works after.

@evanlucas

This comment has been minimized.

Copy link
Member

commented Nov 20, 2015

Yea, apologies on that...not sure how that was missed :/

LGTM

@jscissr

This comment has been minimized.

Copy link
Contributor Author

commented Nov 21, 2015

I'll try making a test

@jasnell

This comment has been minimized.

Copy link
Member

commented Nov 21, 2015

It's not critical, just helpful. :) thank you!
On Nov 20, 2015 4:50 PM, "Jan Schär" notifications@github.com wrote:

I'll try making a test


Reply to this email directly or view it on GitHub
#3946 (comment).

@jscissr

This comment has been minimized.

Copy link
Contributor Author

commented Nov 21, 2015

Hmm, it looks like req.localAddress and req.localPort isn't even set anywhere, so the if block will never execute...
Should I add them after https://github.com/jscissr/node/blob/patch-2/lib/net.js#L819 ?

At least it means the worst that can happen is that you don't get a nice error message in the onerror event.

net: add localAddress/Port for better error msgs
The code generating the errror messages was already there, but it could
never execute because these properties were not set.
var net = require('net');

var client = net.connect({
port: common.PORT + 1,

This comment has been minimized.

Copy link
@jscissr

jscissr Nov 21, 2015

Author Contributor

I'm not sure which port to use here, it simply should produce an ECONNREFUSED error when connecting.

lib/net.js Outdated
}
var ex = exceptionWithHostPort(status,
'connect',
req.address,
req.port,
details);
if (req.localAddress && req.localPort) {

This comment has been minimized.

Copy link
@evanlucas

evanlucas Nov 21, 2015

Member

at this point, you could change the conditional to be if (details) {

@@ -0,0 +1,21 @@
'use strict';
var common = require('../common');

This comment has been minimized.

Copy link
@evanlucas

evanlucas Nov 23, 2015

Member

mind making these const instead of var?

This comment has been minimized.

Copy link
@jscissr

jscissr Nov 23, 2015

Author Contributor

I copied that from another test. Should I make a PR that changes this for every test where it isn't already the new way?

This comment has been minimized.

Copy link
@evanlucas

evanlucas Nov 23, 2015

Member

I believe the plan is to update them as we go. But in general, all new tests should use const wherever possible

});

var onErrorCalled = false;
client.on('error', function(err) {

This comment has been minimized.

Copy link
@evanlucas

evanlucas Nov 23, 2015

Member

instead of having the onErrorCalled, you could wrap the callback to the error event in common.mustCall().

You could then remove the process.on('exit') listener too

var client = net.connect({
port: common.PORT + 1,
localPort: common.PORT,
localAddress: '127.0.0.1'

This comment has been minimized.

Copy link
@cjihrig

cjihrig Nov 23, 2015

Contributor

Instead of hard coding the IP address, can you use common.localhostIPv4.

@cjihrig

This comment has been minimized.

Copy link
Contributor

commented Nov 23, 2015

LGTM pending comments and CI

@jscissr jscissr changed the title net: variable ex used before defined net: add localAddress/Port for better error msgs Nov 23, 2015

@evanlucas

This comment has been minimized.

Copy link
Member

commented Nov 23, 2015

@jscissr

This comment has been minimized.

Copy link
Contributor Author

commented Nov 24, 2015

It looks like something was wrong with the CI server. Can you restart it @evanlucas?

@evanlucas

This comment has been minimized.

evanlucas added a commit that referenced this pull request Nov 24, 2015

net: add local address/port for better errors
Adds localAddress and localPort to req so we have better error messages.
Also fixes a case where ex is used before it is declared.

PR-URL: #3946
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@evanlucas

This comment has been minimized.

Copy link
Member

commented Nov 24, 2015

Landed in d7b199d. I went ahead and squashed the commits into 1 also. Thanks!

@evanlucas evanlucas closed this Nov 24, 2015

@jscissr jscissr deleted the jscissr:patch-2 branch Nov 25, 2015

MylesBorins added a commit that referenced this pull request Dec 1, 2015

net: add local address/port for better errors
Adds localAddress and localPort to req so we have better error messages.
Also fixes a case where ex is used before it is declared.

PR-URL: #3946
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

rvagg added a commit that referenced this pull request Dec 4, 2015

net: add local address/port for better errors
Adds localAddress and localPort to req so we have better error messages.
Also fixes a case where ex is used before it is declared.

PR-URL: #3946
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

rvagg added a commit that referenced this pull request Dec 5, 2015

net: add local address/port for better errors
Adds localAddress and localPort to req so we have better error messages.
Also fixes a case where ex is used before it is declared.

PR-URL: #3946
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

@jasnell jasnell referenced this pull request Dec 17, 2015

Closed

Release 4.2.4 Planning #4321

jasnell added a commit that referenced this pull request Dec 17, 2015

net: add local address/port for better errors
Adds localAddress and localPort to req so we have better error messages.
Also fixes a case where ex is used before it is declared.

PR-URL: #3946
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

jasnell added a commit that referenced this pull request Dec 23, 2015

net: add local address/port for better errors
Adds localAddress and localPort to req so we have better error messages.
Also fixes a case where ex is used before it is declared.

PR-URL: #3946
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.