Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch http_parser to llhttp #3561

Closed
socketpair opened this issue Jan 21, 2019 · 10 comments · Fixed by #5364 or #6144
Closed

Switch http_parser to llhttp #3561

socketpair opened this issue Jan 21, 2019 · 10 comments · Fixed by #5364 or #6144

Comments

@socketpair
Copy link
Contributor

socketpair commented Jan 21, 2019

Nodejs decide to move from their hard-coded C parser to this one.

nodejs/http-parser#285 (comment)

Upd:
proposed parser is https://llhttp.org

@aio-libs-bot
Copy link

GitMate.io thinks the contributor most likely able to help you is @asvetlov.

Possibly related issues are #33 (HTTPS Support), #390 (404 on http://aiohttp.readthedocs.org/en/v0.16.3/), #320 (HTTP/2 support), #72 (HTTP POST not working), and #858 (replace http parser?).

@samuelcolvin
Copy link
Member

Seems too good to be true, but must be worth trying.

Might be worth putting the link in the body of the issue so it's rendered as a link.

@socketpair
Copy link
Contributor Author

https://llhttp.org

@socketpair socketpair changed the title Switch to https://llhttp.org/ Switch http_parser to llhttp Jan 21, 2019
@socketpair socketpair removed the bug label Jan 21, 2019
@asvetlov
Copy link
Member

It worth to try.
API seems the same but I expect complications of our build process.
Don't have any personal estimation of the issue:

  1. It would be nice to have but not urgent, existing parser just works
  2. I have a long list of higher priority items in my todo list

@socketpair
Copy link
Contributor Author

Interesting, suricata decide to write their own HTTP-parser on Rust :) https://redmine.openinfosecfoundation.org/issues/2787#change-10814

@derlih
Copy link
Contributor

derlih commented Dec 20, 2020

I found a problem with this port to llhttp: there is no API in llhttp that parses url.
These functions are needed by cdef class HttpRequestParser(HttpParser)
Does anyone have an idea how to workaround it?

my llhttp branch

@asvetlov
Copy link
Member

They are not required I guess, yarl.URL can be constructed directly as we do for pure-Python parser.

@derlih
Copy link
Contributor

derlih commented Dec 22, 2020

@socketpair
I saw this project, but

This project is a toy, started to reacquaint myself with Python c-api modules.

@derlih derlih mentioned this issue Dec 22, 2020
5 tasks
@derlih derlih linked a pull request Dec 22, 2020 that will close this issue
5 tasks
@socketpair
Copy link
Contributor Author

socketpair commented Dec 22, 2020

@derlih That's why I removed the link :) before your comment.

@derlih derlih removed a link to a pull request Dec 22, 2020
5 tasks
@socketpair
Copy link
Contributor Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants