search

nodejs / http-parser

http request/response parser for c
MIT License
6.32k stars 1.53k forks source link

Handle URLs with a colon after host but no port #501

Open nico202 opened 4 years ago

nico202 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

sam-github commented 4 years ago

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 commented 4 years ago

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:

sam-github commented 4 years ago

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 commented 4 years ago

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

nico202 commented 4 years ago

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

nico202 commented 4 years ago

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