Please someone mege this?

Hey!

Thank you for submitting this and sorry I didn't get back to you earlier.

Adding new HTTP methods is tricky in HTTP 1.1. According to the protocol different methods might imply presence or absence of body in the request/response. It is not possible to apriori know it without hand-coding this semantic into it.

Secondly, and likely more importantly with this patch the burden of parsing the HTTP method is shifted to the user of the library, and the optimization (happening in llparse) is not utilized for method parsing at all. I don't think this is the direction we want to go to with this library.

With this being said, I suggest we close the Pull Request. I wish you would have had a chance to discuss it with us before you created this sizable patch. Sorry to dismiss you work, but I don't think it is in line with the idea of the llhttp.

Thanks again!

Hi @indutny,
I've created a patch for my own use, i thought that could be useful to the community, for this reason I've not discussed it before submitting.

Talking about HTTP 1.1, the RFC 7231 Section 4.1 does not limit HTTP method and defines it as

    method = token

Some methods are standardized in the same RFC, but there's no RFC that restricts methods. The only requirement is that the method MUST contain only characters in US-ASCII charset.

The same RFC also explicit (in section 8.1.2) that

   Since message parsing (Section 3.3 of [RFC7230]) needs to be
   independent of method semantics (aside from responses to HEAD),
   definitions of new methods cannot change the parsing algorithm or
   prohibit the presence of a message body on either the request or the
   response message.

and

   To allow existing parsers to process the
   response message, new status codes cannot disallow a payload,
   although they can mandate a zero-length payload body.

According to this section, excluding GET and HEAD, all the other method including the non-standardized ones must not prohibit the body.

Secondly, and likely more importantly with this patch the burden of parsing the HTTP method is shifted to the user of the library, and the optimization (happening in llparse) is not utilized for method parsing at all.

Regarding this: it's true, unfortunately, but without a method map, it's unavoidable. In addition, this allows to parse HTTP-like messages, but with another protocol (RTSP for example). For this case the method map is not usable and is removed in the patch.

I don't think this is the direction we want to go to with this library.

I think that a roadmap, known issues that will not be fixed or at least the limits that this library will not trespass could help to choose this library as base tool or another parsing library.

IMHO the direction was not clear enough, but if this direction is confirmed, I will personally start evaluating another library for my projects or create a fork.

Thank you for an elaborated write-up. Your reasoning behind the patch is very sound.

I'd like to invite few members of @nodejs/http here for discussion. There was never a specific roadmap or direction anywhere, but in my head prior to this, but I'm open to improving things!