Giter Site home page Giter Site logo

Update http_parser about crow HOT 16 CLOSED

The-EDev avatar The-EDev commented on May 18, 2024
Update http_parser

from crow.

Comments (16)

The-EDev avatar The-EDev commented on May 18, 2024 1

Update: looking at the changes in the h file, the biggest change seems to be that HTTP codes have been added, on the c files however, there are many changes, 99% semantic, but 1% could be useful

Suggestion: instead of upgrading, only update the relevant parts.
Suggestion 2: instead of upgrading or updating, replace the out of maintenance http_parser library with this parser from Nginx, it's worth noting this is the parser that the nodeJS library is based on.

from crow.

The-EDev avatar The-EDev commented on May 18, 2024 1

@rijojosephc Yes I disregarded it when I saw that 72% was TypeScript. Let me know if i'm wrong and the 72% is just interface, with a C library similar to http-parser still being there.

In any case, migrating to any parser, be it Nginx or llhttp (without @ipkn's help) requires quite a large effort due to the fact that http_parser.h has been modified quite a bit and is no longer directly compatible even with the original nodejs/http-parser.

from crow.

luca-schlecker avatar luca-schlecker commented on May 18, 2024 1

On the one side, it would be somewhat convenient to have a kind of all-in-one package, but Crow's main purpose is to provide a server and I'm not quite sure how often one need's to get another resource on the server-side. If one really needed that, he could still use cpr.

from crow.

The-EDev avatar The-EDev commented on May 18, 2024

ok so a bit of history:

  1. Crow was not always header only.
  2. Crow used to use http_parser as a submodule (back when the repo was joynet/http_parser).
  3. Crow became header only, at that time @ipkn merged http_parser into 1 header file.
  4. Between 3 and 2017, http_parser.h was modified, moved into the include forlder, and some other stuff.

The clear change is adding PATCH and PURGE methods, simple enough, what's not simple is that XX was renamed to CROW_XX and there's no commit mentioning it.

2 things are required to get http_parser upgraded:

  1. Figure out how to merge the whole repo into 1 header file.
  2. get a diff between the 2 files and if a change seems to be made by @ipkn, apply it in the new file.

from crow.

mrozigor avatar mrozigor commented on May 18, 2024

I think second suggestion is the best.

from crow.

rijojosephc avatar rijojosephc commented on May 18, 2024

@The-EDev : Have we already considered the possibility of migrating to llhttp which claims to be the better maintained port of http-parser itself?

from crow.

rijojosephc avatar rijojosephc commented on May 18, 2024

@The-EDev In the README of llhttp I saw the usage with #include "llhttp.h".
I have not tried llhttp yet. Just wanted to check if you have already tried it.

If http_parser.h is a heavily modified version of the original, then it will indeed be quite a bit of effort to "migrate".

from crow.

The-EDev avatar The-EDev commented on May 18, 2024

Going through the README, it seems that the parser is written in TypeScript then compiled into C. Something about that rubs me the wrong way.

I don't mind generating code that is tedious to write (see mime_types.h). but the whole HTTP parser? I don't know if I like that idea..

from crow.

The-EDev avatar The-EDev commented on May 18, 2024

Looking at Nginx's Http parser, I can see a few ways of migrating crow to it.

The first is to take ngx_http_parse.c and any dependencies it has, then slowly carve out any pieces not required for crow to operate and then changing all the ngx_ names, then use crow's parser to interface with nginx's parser and convert nginx's request to a crow request. This is the easier route made slightly more difficult by the lack of documentation on how @ipkn managed to do it with the original parser.

The second is to take ngx_http_parse.c and replace any dependencies or parts to which a crow alternative exists with said crow alternative (e.g. replace ngx_http_request_t with crow::request and adapt nginx's parser to the change). This will in the short term downgrade crow's http-parser wrapper to just a simple interface and in the long term completely remove it.

A third option where the nginx extras are taken out of ngx_http_parse.c and the remaining file changed to adapt to the existing http-parser wrapper is possible, and is a good balance between the first 2 options.

I personally prefer option 2, but am still open to discussion.

from crow.

The-EDev avatar The-EDev commented on May 18, 2024

Alright first update, The new parser is in a state of messiness, but I think I have the basics down. I'm assuming I'll need only 2 of the 10 functions included (parse request line and parse header) the rest are for 3 kinds of URIs (already neing done in router), Cookies (being done in middleware), arguments (being done in qs_parse), responses (not needed because crow's not a client), chunked requests (should investigate) and multi line headers (should also investigate).

The plan for now is to just get the thing working (primarily by replication nginx's data structures and later converting them to crow structures) and then move on to phasing out everything nginx and using crow's data structures.

from crow.

The-EDev avatar The-EDev commented on May 18, 2024

Testing for ngx_http_parse_request_line (parses the very first line of a request) seems to be working flawlessly.

Moving on to test ngx_http_parse_header_line.

UPDATE: ngx_http_parse_header_line works flawlessly as well. The next step is to link the new parser to the wrapper.

from crow.

The-EDev avatar The-EDev commented on May 18, 2024

Day 3 of working on this:
The Parser wrapper is a shell of its former self, I expect it to be completely phased out by the time a PR is made, or by the next PR relating to the parser.

I was able to successfully create a crow::request object with all the correct data (I've not tested parameters yet, but they should work too).
I'll keep the wrapper for now, just to keep the http_connection code undisturbed. And will try to get the code in a state where I can run unit tests.

UPDATE: Hello world example works, unit tests hang. next step is to get them working, and start trimming unnecessary parts

from crow.

The-EDev avatar The-EDev commented on May 18, 2024

I finally managed to get through the brick wall that is the unit tests (only the first problematic unit test actually), I had to revamp the way the parser works to support messages being sent in pieces, only thing left is parsing the body in a way where content-length is taken into account (so that a request isn't processed until the full body is sent)

from crow.

The-EDev avatar The-EDev commented on May 18, 2024

The new parser is proving extremely difficult to set up, sure it works most of the time.. but there are multiple random issues that in all honesty make no sense to me. I might have to change my approach, cutting unnecessary parts from the existing parser while making sure everything works until the transition from old to new is as smooth as can be

from crow.

The-EDev avatar The-EDev commented on May 18, 2024

I went through the old parser and found a few pieces of code that could be taken out, most of them relating to response parsing. Taking these parts out will do 2 things:

  1. Set up the internal parts of the parser to be replaced with their Nginx counterparts.
  2. Make it extremely difficult to add HTTP client support to Crow (as in Crow making requests).

I'm torn on this subject, @mrozigor, @luca-schlecker What do you guys think? Are we ever going to add something like running Crow as a client? Or should I go ahead and modify the parser?

from crow.

The-EDev avatar The-EDev commented on May 18, 2024

Update:
I thought it would be best to upgrade the parser to v2.9.4 (the latest before the parser became unmaintained) before making any changes. And once that's done I can integrate it more with Crow and possibly either keep the modified version or swap it with another parser (most likely Nginx's).

So far I managed to go through about 40%-50% of the commits since the parser was integrated into Crow. I've reached version v2.6.0, and skipped 6 commits (either for solving problems already solved in Crow or completely breaking compatibility with the wrapper).

Another Update:
I reached v2.7.1 (commit 1b79abab34d4763c0467f1173a406ad2817c1635) with almost no trouble, This is significant (at least sentimentally) because that is the last commit that affects Crow which was published before it was abandoned in 2017, any new additions beyond this will be beyond the original repository's updates.

from crow.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.