Handle URLs with a colon after host but no port

[nico202]

Hello!

According to this issue on libgit2,
RFC 3986 says:

URI producers and normalizers should omit the port component and its
":" delimiter if port is empty or if its value would be the same as
that of the scheme's default.

They are patching http-parser in order to support it. A similar patch (deleting case s_http_host_port_start: around line 2394) works fine also on latest http-parser release (v2.9.3), but a test is broken
*** http_parser_parse_url("http://hostname:/") "proxy empty port" test failed, unexpected rv 0 ***

Merging something similar on the main repo would prevent maintainers from varios distros to apply the patch by themeselves.

Can something similar be merged here?

Thanks,
Nicolò

[sam-github]

Could you track down the actual patch that is being applied to the http_parser and link to it? None of the links above lead to it, they just mention it exists, somewhere.

[nico202]

sorry, it was linked in last link thread. Here you go:
libgit2/libgit2@1bbdec6

[sam-github]

Ouph. No tests. I'd say "libgit2 should move to llhttp", but that doesn't have a URL parser ;-).

I don't personally object to the change, if it matches RFC, but I'm not sure I have the time to make it, not in the next weeks anyhow. It would require tests to land, and we'd have to run the Node.js tests against it to make sure its sufficiently backwards compatible. Maybe benchmarks are not necessary, given the nature of the single-line deletion.

@indutny @bnoordhuis Thoughts?

@nico202 Are you interested in PRing the change?

[nico202]

They test it, but with their test suite
https://github.com/libgit2/libgit2/pull/5108/files#diff-c5c99906c1debd58788c8079d0eff94c

I can delete the line and fix tests accordingly in the following days/this week end.

I can run tests for some packages depending on http-parser available in guix

• [email protected]
• [email protected]
• [email protected]
• [email protected]
  but it depends on how fast my server goes.

If other members are fine with this, I'll try to patch & test 👍

[sam-github]

This repo is currently "lightly maintained"... I'm not sure how quickly you can get a definitive answer. Sorry. But PRed code usually gets more comment than a statement of intent.

I don't have power to approve or merge, but as a Node.js downstream maintainer, I could probably block it if it broke node, or negatively affected performance ;-P

[nico202]

Yeah fine. I've built node, and tests are fine!
So I'll PR soon

[nico202]

Benchmark (bench.c, on an old laptop, 2 core, 8Gb memory, 2 run) seems to be fine.
current:

• 8192.00 mb | 126.45 mb/s | 255971.06 req/sec | 64.78 s
• 8192.00 mb | 126.99 mb/s | 257063.22 req/sec | 64.51 s
  patched:
• 8192.00 mb | 126.71 mb/s | 256489.97 req/sec | 64.65 s
• 8192.00 mb | 126.81 mb/s | 256696.81 req/sec | 64.60 s

[nico202]

Wait, I think I'm just stupid and missed this:
#483

It's already been done, it just need to be merged