HttpParserUpgrade raised after event hooks are called.

[tomchristie]

The HttpParserUpgrade exception appears to get called after the event hooks have been called into (at least with small requests that fit into a single data_received call)

This makes it not terribly useful, as we may have already called into a handler function / issued a response / etc... by the time the upgrade is raised.

Sketch of protocol class:

def __init__(self, ...):
    self.request_parser = httptools.HttpRequestParser(self)
    ...

    # The asyncio.Protocol hooks...
    ...

    def data_received(self, data):
        print('data_received()')
        try:
            self.request_parser.feed_data(data)
        except httptools.HttpParserUpgrade:
            print('upgrade exception')

    # Event hooks called back into by HttpRequestParser...
    ...

    def on_headers_complete(self):
        print('on_headers_complete()')

    def on_message_complete(self):
        print('on_message_complete()')

Output:

data_received()
on_headers_complete()
on_message_complete()
upgrade exception

On first sight this looks awkward to resolve, so it could just be considered a constraint of the implementation, and deal with it in python-land instead (which would be acceptable from my POV)

[tomchristie]

My current approach is:

• Inspect upgrade header in on_header.
• Ignore any events in on_body and on_message complete if an upgrade header existed.
• Handle the httptools.HttpParserUpgrade in data_received as normally.

That works fine, only downside being a small amount of extra work in Python-land that could perhaps be dealt with by the parser, if the exception was raised prior to the event hooks being called.

I guess good options would be:

• Close this off as a known constraint.
• Ensure that httpparser instead raises the exception immediately prior to the point of calling on_headers_complete.

[tomchristie]

Okay, looks like this is a limitation of the underlying library...

the parser will treat this as a normal HTTP message without a body, issuing both on_headers_complete and on_message_complete callbacks

I'll close the issue off.

[ramonz]

hi. also got HttpParserUpgrade when parsing
b'CONNECT /proxy/url HTTP/1.1\r\nHost: test01-proxy.com:80\r\nAuthorization: Basic dXNlcjpwYXNz\r\nX-Forwarded-For: 8.8.8.8\r\n\r\n'

In [60]: b = b'CONNECT /proxy/url HTTP/1.1\r\nHost: test01-proxy.com:80\r\nAuthorization: Basic dXNlcjpwYXNz\r\nX-Forwarded-For: 8.8.8.8\r\n\r\n'

In [61]: p = HttpRequestParser (lambda: None)

In [62]: p.feed_data(b)
---------------------------------------------------------------------------
HttpParserUpgrade                         Traceback (most recent call last)
<ipython-input-62-032eaec4cf5a> in <module>()
----> 1 p.feed_data(b)

httptools/parser/parser.pyx in httptools.parser.parser.HttpParser.feed_data (httptools/parser/parser.c:2781)()

HttpParserUpgrade: 119

if replace the b'\r\n\r\n' with b\r\n' the error is disappears

made more tests. parsing the request above and even replacing '\r\n\r\n' not works correctly: callback functions such as on_message_complete, on_headers_complete not fired at all.
just replacing the b'CONNECT' with b'GET' without other modifications makes all working.

[yohanboniface]

cf #29 for the record

[yohanboniface]

@ramonz I think it's expected behaviour, see https://github.com/nodejs/http-parser/blob/0264a0aefcd119e179a3bc730ac517efbbccd4bb/http_parser.c#L1319