http-parser icon indicating copy to clipboard operation
http-parser copied to clipboard

Published 20 hours ago • verified publisher icon nodejs

Handle URLs with a colon after host but no port

Open nico202 opened this issue 4 years ago • 8 comments

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ò

nico202 avatar Mar 17 '20 13:03 nico202

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.

sam-github avatar Mar 17 '20 15:03 sam-github

sorry, it was linked in last link thread. Here you go: https://github.com/libgit2/libgit2/pull/5108/commits/1bbdec69bef50208f77f0c4cbac7c6b56c35973f

nico202 avatar Mar 17 '20 15:03 nico202

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?

sam-github avatar Mar 17 '20 15:03 sam-github

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

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

nico202 avatar Mar 17 '20 16:03 nico202

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

sam-github avatar Mar 17 '20 16:03 sam-github

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

nico202 avatar Mar 17 '20 17:03 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 avatar Mar 17 '20 18:03 nico202

Wait, I think I'm just stupid and missed this: https://github.com/nodejs/http-parser/pull/483

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

nico202 avatar Mar 17 '20 18:03 nico202