llhttp-v3.0.0 lost on_header_field_complete event

[ldcsaa]

These is my code (VC 2010)

When I run this program, 'Content-Length' and 'Connection' header does NOT trigger _on_header_field_complete event. (PS: 'Content-Type' and 'User-Agent' header does trigger on_header_field_complete_ event )

LPCSTR TEST_RESP = "HTTP/1.1 200 HP Http Server OK\r\n"
           "Content-Type: text/plain\r\n"
           "Content-Length: 20\r\n"
           "User-Agent: IE9\r\n"
           "Connection: keep-alive\r\n"
           "\r\n"
           "[test datas: XXXXXX]";

llhttp_t g_parser;
llhttp_settings_t g_settings;

void TestResp()
{
    size_t len = strlen(TEST_RESP);
    llhttp_errno err = llhttp_execute(&g_parser, TEST_RESP, len);

    if(err == HPE_OK)
    {
        err = llhttp_finish(&g_parser);
    }
}

void InitParser()
{
    llhttp_settings_init(&g_settings);

    g_settings.on_message_begin     = on_message_begin;
    g_settings.on_url           = on_url;
    g_settings.on_url_complete      = on_url_complete;
    g_settings.on_status            = on_status;
    g_settings.on_status_complete       = on_status_complete;
    g_settings.on_header_field      = on_header_field;
    g_settings.on_header_field_complete = on_header_field_complete;
    g_settings.on_header_value      = on_header_value;
    g_settings.on_header_value_complete = on_header_value_complete;
    g_settings.on_headers_complete      = on_headers_complete;
    g_settings.on_body          = on_body;
    g_settings.on_message_complete      = on_message_complete;
    g_settings.on_chunk_header      = on_chunk_header;
    g_settings.on_chunk_complete        = on_chunk_complete;

    llhttp_init(&g_parser, HTTP_BOTH, &g_settings);
}

int _tmain(int argc, _TCHAR* argv[])
{
    InitParser();
    TestResp();

    return 0;
}

[indutny]

Looks like coverage in 4cc13c09 wasn't exhaustive. @pallas : could you take a look at it?

[pallas]

Yes, I can take a look. These are both "special" headers, so I probably just missed a path they're in. Note: I am also very not proficient with Node.js or llparse.

[pallas]

Also, this is a new feature, so no reason to roll back: it doesn't break anything that isn't trying to use the new complete callbacks, those callbacks are just missing something.

[indutny]

@pallas totally agree. This can be fixed without reverts. If you have any questions or if something is looking confusing - let me know! I'd love to help/collaborate.

[pallas]

@ldcsaa can you please try with the above patch?

[pallas]

@indutny thanks for the offer, I think I untangled it all.. it was a pretty obvious mistake on my part in retrospect and it would have been caught if I had done better testing

[ldcsaa]

@ldcsaa can you please try with the above patch?

I had test it, the bug has fixed ~

[pallas]

Awesome! Glad someone else is using this feature, sorry for the bug in the first place.

[dccarter]

I came across this issue when trying to parse the following request and figured there is a fix on the way.

GET /home?name=Carter&age=6 HTTP/1.1\r\n
Content-Length: 11\r\n
Connection: close\r\n
Foo: bar\r\n\r\n
Hello World

When handling the on_message_complete shouldn't llhttp_t.content_length be set to 11?

[indutny]

@dccarter it would be 11 in on_headers_complete complete callback, but the field value is undefined at the time of on_message_complete (it is used internally after on_headers_complete)

[dccarter]

@dccarter it would be 11 in on_headers_complete complete callback, but the field value is undefined at the time of on_message_complete (it is used internally after on_headers_complete)

Thanks for quick response