http2: allow port 80 in http2.connect#16337
Conversation
There was a problem hiding this comment.
I can confirm that this fixes the #14304 case for me. All tests are OK for the local Windows 7 x64 build.
|
Single CI failure is unrelated. |
There was a problem hiding this comment.
Instead of calling process.exit() here, can you let the test exit naturally.
There was a problem hiding this comment.
Then I need to do a bunch of error handling for the about to fail http2.connect since I'm not going to return an actual socket. That seemed worse to me because then it becomes super reliant on the exact code within that function (or I need to wrap it in assert.throws and that also seemed super sketchy).
This was the least brittle and least reliant on the exact internals of http2.connect solution that I could come up with. Definitely open to alternatives though!
I suppose I could throw a custom error within net.connect and then catch that outside... is that really better? I don't really know.
There was a problem hiding this comment.
I definitely wouldn't call it "super sketchy" but I get what you mean. Can you try to keep that in mind for the future though. We generally try to avoid process.exit() in tests.
There was a problem hiding this comment.
I feel like any solution I come up with here is a bit sketchy haha. I don't like process.exit but it's less convoluted than the other alternative I can immediately come up with, which is:
I suppose I could throw a custom error within net.connect and then catch that outside.
If most people prefer that, I'm happy to refactor.
There was a problem hiding this comment.
@apapirovski how about something like this:
const original = net.connect;
net.connect = common.mustCall((...args) => {
assert.strictEqual(args[0], '80');
return original(...args);
});
// Use a non-routable IP address.
const client = http2.connect('http://192.0.2.1:80');
client.destroy();Due to how WHATWG-URL parser works, port numbers are omitted if they are the default port for a scheme. This meant that http2.connect could not accept connections on port 80 with http scheme. Fix this bug by detecting http: scheme and setting port to 80. Fixes: nodejs#14304
febaddd to
e756215
Compare
|
Landed in c30f107 |
Due to how WHATWG-URL parser works, port numbers are omitted if they are the default port for a scheme. This meant that http2.connect could not accept connections on port 80 with http scheme. Fix this bug by detecting http: scheme and setting port to 80. PR-URL: #16337 Fixes: #14304 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
Due to how WHATWG-URL parser works, port numbers are omitted if they are the default port for a scheme. This meant that http2.connect could not accept connections on port 80 with http scheme. Fix this bug by detecting http: scheme and setting port to 80. PR-URL: #16337 Fixes: #14304 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
Due to how WHATWG-URL parser works, port numbers are omitted if they are the default port for a scheme. This meant that http2.connect could not accept connections on port 80 with http scheme. Fix this bug by detecting http: scheme and setting port to 80. PR-URL: nodejs/node#16337 Fixes: nodejs/node#14304 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
Due to how WHATWG-URL parser works, port numbers are omitted if they are the default port for a scheme. This meant that http2.connect could not accept connections on port 80 with http scheme. Fix this bug by detecting http: scheme and setting port to 80. PR-URL: nodejs/node#16337 Fixes: nodejs/node#14304 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
Due to how WHATWG-URL parser works, port numbers are omitted if they are the default port for a scheme. This meant that
http2.connectcould not create connections on port 80 with http scheme. Fix this bug by detectinghttp:scheme and setting port to 80.Fixes: #14304
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
http2, test