test: remove common.PORT from test-tls-connect #12508
Conversation
Replace common.PORT with 0 to prevent the occurence of any EADDRINUSE errors. Refs: nodejs#12376
| @@ -37,7 +37,7 @@ const path = require('path'); | |||
| const cert = fs.readFileSync(path.join(common.fixturesDir, 'test_cert.pem')); | |||
| const key = fs.readFileSync(path.join(common.fixturesDir, 'test_key.pem')); | |||
|
|
|||
| const options = {cert: cert, key: key, port: common.PORT}; | |||
| const options = {cert: cert, key: key, port: 0}; | |||
| const conn = tls.connect(options, common.mustNotCall()); | |||
|
|
|||
| conn.on('error', common.mustCall()); | |||
There was a problem hiding this comment.
Now that you are doing this, it would be better if you could assert the actual error messages. On my machine I got
{ Error: connect ECONNREFUSED 127.0.0.1:12346
at Object.exports._errnoException (util.js:1042:11)
at exports._exceptionWithHostPort (util.js:1065:20)
at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1115:14)
code: 'ECONNREFUSED',
errno: 'ECONNREFUSED',
syscall: 'connect',
address: '127.0.0.1',
port: 12346 }
There was a problem hiding this comment.
Using port: 0 I got the same on Linux, however the error code changes to EADDRNOTAVAIL on OS X for some reason.
There was a problem hiding this comment.
Both of these changes use port 0 in tls.connect(). That doesn't work, does it? (I get how it assigns an arbitrary available port .bind() or .listen() but .connect()?) and we may be breaking the test now. These connections are supposed to fail for other reasons, but now they might be failing because we're giving an invalid port number for a connection.
|
@Trott Using port [1] /~https://github.com/torvalds/linux/blob/master/net/ipv4/inet_connection_sock.c#L278-L280 |
Much to my surprise, that appears to be correct. We should probably document this behavior if it's intentional. (We should also make sure that it works this way on Windows.) I'll clear my review so it doesn't block this. Might be worth adding a comment along the lines of: // It's not documented in the Node.js docs, but sending port 0 to `tls.connect()`
// results in attempting to connect to an unused port. |
There was a problem hiding this comment.
Argh, hate to flip-flop again, but more testing makes me skeptical this works as advertised again. Port 0 does not attempt to initiate a connection at all. It returns an error immediately. If I provide an unreachable address and a valid port number, I get ENETUNREACH after a pause. If I provide an unreachable address and port 0, I get EADDRNOTAVAIL immediately. Using port 0 with connect() does not seem to return an arbitrary port unless it is special-cased for localhost. (Perhaps it is, but I'd want confirmation in the code or through testing...)
|
More evidence it does not choose an arbitrary port: If I use localhost and provide a port not in use, I get |
|
Yup, I was able to reproduce the same. Haven't been able to test on Windows though, so can't say what the situation is there. Probably worth adding more coverage to the errors returned. |
|
As there was no update for a long time and it seems like it is inconclusive if this actually works or not, I am going to close this. @tanuck Thanks a lot for your contribution anyway and please feel free to reopen this if you would like to follow up on it! |
Replace common.PORT with 0 to prevent the occurence of
any EADDRINUSE errors.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
test tls https