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

tls: prefer path over port in connect #14564

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
8 participants
@bengl
Copy link
Member

commented Aug 1, 2017

Makes tls.connect() behave as documented, preferring options.path over
options.port. This makes it consistent with net.connect(), so the
included test demonstrates that both behave in this way.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

tls, test

@bengl

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2017

@mscdex

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2017

Is this semver-major?

@bengl

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2017

Yes most likely semver major.

@bengl bengl added the semver-major label Aug 1, 2017

@jasnell

jasnell approved these changes Aug 1, 2017

Copy link
Member

left a comment

LGTM with a CITGM run.

@jasnell

This comment has been minimized.

Copy link
Member

commented Aug 8, 2017

ping @nodejs/ctc

@jasnell jasnell requested a review from Aug 8, 2017

@BridgeAR

This comment has been minimized.

Copy link
Member

commented Aug 27, 2017

If it is documented that path is preferred over port and it is also consistent towards net.connect(), I would argue it is a semver-patch. Especially as the options are not compatible with each other and should normally not coexist in the first place.

path: unixServer.address(),
port: tcpServer.address().port,
host: 'localhost',
rejectUnauthorized: false,

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Aug 29, 2017

Contributor

Nit: extra comma at the end.

});
}

testLib(net, () => testLib(tls, common.mustCall()));

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Aug 29, 2017

Contributor

Callback for net case also should have common.mustCall

});
}
const server = lib.createServer(...args);
server.listen(tcp ? 0 : common.PIPE, () => cb(server));

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Aug 29, 2017

Contributor

common.mustCall for this callback also?

rejectUnauthorized: false,
}, () => {
const bufs = [];
client.on('data', common.mustCall((d) => {

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Aug 29, 2017

Contributor

Generally, data handler will be called multiple times. If we are sure that it will be called only once, then bufs need not be an array, right?

This comment has been minimized.

Copy link
@bengl

bengl Sep 20, 2017

Author Member

I don't think there's a reason to be sure it will be only called once, is there?

assert.strictEqual(`${libName(lib)}:${unixServer.address()}`, resp);
tcpServer.close();
unixServer.close();
if (cb) {

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Aug 29, 2017

Contributor

All the tests pass a callback function. Why do we need this check?

lookup: options.lookup
};
}
const connect_opt = {

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Aug 29, 2017

Contributor

Nit: I know that this uses the same variable name as the old one, but most part of the core uses camel case. Could you change this?

};
}
const connect_opt = {
path: options.path,

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Aug 29, 2017

Contributor

I would prefer a documentation change, which explicitly states that if path is present, that gets first preference.

This comment has been minimized.

Copy link
@bengl

bengl Sep 20, 2017

Author Member

That's already stated in the tls doc. I'll add it to net.

@bengl bengl referenced this pull request Aug 29, 2017

Closed

http: client keep-alive for UNIX domain sockets #13214

3 of 3 tasks complete
@mcollina
Copy link
Member

left a comment

LGTM with @thefourtheye nits addressed

@BridgeAR

This comment has been minimized.

Copy link
Member

commented Sep 13, 2017

Ping @bengl

@bengl

This comment has been minimized.

Copy link
Member Author

commented Sep 14, 2017

Whoops! Got swamped with product work and let this slide. I'll try and clean this up in the next few days.

tls: prefer path over port in connect
Makes tls.connect() behave as documented, preferring options.path over
options.port. This makes it consistent with net.connect(), so the
included test demonstrates that both behave in this way.

Also, for consistency, noting the precedence of options.path in net
doc.

@bengl bengl force-pushed the bengl:bengl/consistentconnect branch from bbc9851 to 27cf4d2 Sep 20, 2017

@bengl

This comment has been minimized.

Copy link
Member Author

commented Sep 20, 2017

@thefourtheye latest commit addresses most of your nits I think, PTAL. Also note the doc change, as requested.

New CI: https://ci.nodejs.org/job/node-test-pull-request/10152/

@bengl

This comment has been minimized.

Copy link
Member Author

commented Sep 20, 2017

On further thought, I agree with @BridgeAR here, in that this is semver-patch, since it's making the behavior match the existing documentation, while it didn't before. I'll remove the semver-major label.

@bengl bengl removed the semver-major label Sep 20, 2017

@BridgeAR

This comment has been minimized.

Copy link
Member

commented Sep 20, 2017

@nodejs/tsc is everyone fine with this being a semver-patch?

@jasnell

This comment has been minimized.

Copy link
Member

commented Sep 20, 2017

If there are no objections from tsc by friday it should be ok to proceed with semver-patch

@bengl

This comment has been minimized.

Copy link
Member Author

commented Sep 22, 2017

OK, cool. It's Friday. I'll land this shortly.

bengl added a commit that referenced this pull request Sep 22, 2017

tls: prefer path over port in connect
Makes tls.connect() behave as documented, preferring options.path over
options.port. This makes it consistent with net.connect(), so the
included test demonstrates that both behave in this way.

Also, for consistency, noting the precedence of options.path in net
doc.

PR-URL: #14564
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@bengl

This comment has been minimized.

Copy link
Member Author

commented Sep 22, 2017

Landed in 6f1caad.

@bengl bengl closed this Sep 22, 2017

addaleax added a commit to addaleax/ayo that referenced this pull request Sep 23, 2017

tls: prefer path over port in connect
Makes tls.connect() behave as documented, preferring options.path over
options.port. This makes it consistent with net.connect(), so the
included test demonstrates that both behave in this way.

Also, for consistency, noting the precedence of options.path in net
doc.

PR-URL: nodejs/node#14564
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

jasnell added a commit that referenced this pull request Sep 25, 2017

tls: prefer path over port in connect
Makes tls.connect() behave as documented, preferring options.path over
options.port. This makes it consistent with net.connect(), so the
included test demonstrates that both behave in this way.

Also, for consistency, noting the precedence of options.path in net
doc.

PR-URL: #14564
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@mcollina

This comment has been minimized.

Copy link
Member

commented Oct 16, 2017

This broke MQTT.js mqttjs/MQTT.js#699 and mqttjs/MQTT.js#700. It was indeed semver-major.
I do not think we should revert, as the original behavior was undocumented.

@MylesBorins

This comment has been minimized.

Copy link
Member

commented Oct 17, 2017

I've opted to not land this on 6.x due to the behavior change.

@mcollina lmk if you think this should be backported

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.