http2: allow port 80 in http2.connect

[apapirovski]

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 create connections on port 80 with http scheme. Fix this bug by detecting http: scheme and setting port to 80.

Fixes: #14304

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)

http2, test

[vsemozhetbyt]

I can confirm that this fixes the #14304 case for me. All tests are OK for the local Windows 7 x64 build.

[vsemozhetbyt]

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

[apapirovski]

Single CI failure is unrelated.

[jasnell]

Good catch.

[cjihrig]

LGTM with a nit.

[cjihrig]

Instead of calling process.exit() here, can you let the test exit naturally.

[apapirovski]

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.

[cjihrig]

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.

[apapirovski]

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.

[lpinca]

@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();

[apapirovski]

CI to merge: https://ci.nodejs.org/job/node-test-pull-request/10927/

[apapirovski]

Landed in c30f107