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

Regression with llhttp 8.1.1: test_http_response_parser_code_under_100[c-parser-pyloop] #7327

Closed
1 task done
musicinmybrain opened this issue Jun 27, 2023 · 3 comments
Closed
1 task done
Labels

Comments

@musicinmybrain
Copy link
Contributor

musicinmybrain commented Jun 27, 2023

Describe the bug

While testing the python-aiohttp package in Fedora Linux with the new llhttp 8.1.1 release, I found that there is one regression in the test suite compared to llhttp 8.1.0.

I have not looked into this very closely, but I suspect this is related to #7165 and to nodejs/llhttp#217, which was released as in llhttp 8.1.1.

To Reproduce

gh repo clone aio-libs/aiohttp
cd aiohttp
git submodule update --init
python3 -m venv _e
. _e/bin/activate
pip install -e .[speedups]
make test

(Confirm all tests pass.)

Update llhttp to v8.1.1:

diff --git a/.gitmodules b/.gitmodules
index 4a06d737..1e901ef7 100644
--- a/.gitmodules
+++ b/.gitmodules
@@ -1,4 +1,4 @@
 [submodule "vendor/llhttp"]
     path = vendor/llhttp
     url = https://github.com/nodejs/llhttp.git
-    branch = v6.0.6
+    branch = v8.1.1
diff --git a/vendor/llhttp b/vendor/llhttp
index 69d6db20..7e18596b 160000
--- a/vendor/llhttp
+++ b/vendor/llhttp
@@ -1 +1 @@
-Subproject commit 69d6db2008508489d19267a0dcab30602b16fc5b
+Subproject commit 7e18596bae8f63692ded9d3250d5d984fe90dcfb

Now run the tests again:

make clean
make test

Expected behavior

All tests pass.

2698 passed, 20 skipped, 2 deselected, 30 xfailed in 154.61s (0:02:34)

Logs/tracebacks

================================================================================================================== FAILURES ==================================================================================================================
_________________________________________________________________________________________ test_http_response_parser_code_under_100[c-parser-pyloop] __________________________________________________________________________________________

response = <aiohttp._http_parser.HttpResponseParser object at 0x7fd2685c9fc0>

    def test_http_response_parser_code_under_100(response: Any) -> None:
>       msg = response.feed_data(b"HTTP/1.1 99 test\r\n\r\n")[0][0][0]

response   = <aiohttp._http_parser.HttpResponseParser object at 0x7fd2685c9fc0>

tests/test_http_parser.py:698:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

>   raise ex
E   aiohttp.http_exceptions.BadStatusLine: 400, message="Bad status line 'Invalid status code'"


aiohttp/_http_parser.pyx:551: BadStatusLine


### Python Version

```console
$ python --version
Python 3.11.3

aiohttp Version

$ python -m pip show aiohttp
Name: aiohttp
Version: 4.0.0a2.dev0
Summary: Async http client/server framework (asyncio)
Home-page: https://github.com/aio-libs/aiohttp
Author: 
Author-email: 
License: Apache 2
Location: /home/ben/src/forks/aiohttp/_e/lib/python3.11/site-packages
Requires: aiosignal, async-timeout, charset-normalizer, frozenlist, multidict, yarl
Required-by:

multidict Version

$ python -m pip show multidict
Name: multidict
Version: 5.2.0
Summary: multidict implementation
Home-page: https://github.com/aio-libs/multidict
Author: Andrew Svetlov
Author-email: andrew.svetlov@gmail.com
License: Apache 2
Location: /home/ben/src/forks/aiohttp/_e/lib/python3.11/site-packages
Requires: 
Required-by: aiohttp, yarl

yarl Version

$ python -m pip show yarl
Name: yarl
Version: 1.9.2
Summary: Yet another URL library
Home-page: https://github.com/aio-libs/yarl/
Author: Andrew Svetlov
Author-email: andrew.svetlov@gmail.com
License: Apache-2.0
Location: /home/ben/src/forks/aiohttp/_e/lib/python3.11/site-packages
Requires: idna, multidict
Required-by: aiohttp

OS

Fedora Linux 38 (originally found on Rawhide/F39).

Related component

Client

Additional context

No response

Code of Conduct

  • I agree to follow the aio-libs Code of Conduct
@musicinmybrain
Copy link
Contributor Author

Should a two-digit HTTP status be considered valid? If not, the fix may be as simple as changing this aiohttp test to expect an invalid status code. There is now a test in llhttp verifying that one-digit status codes are (now) considered invalid, but no test either way for two-digit ones.

@Dreamsorcerer
Copy link
Member

Hmm, the test looks like it was a deliberate idea to support it in aiohttp. Probably also related to #5269 (comment) which I suspect will allow the value in llhttp (though I've not checked).

@musicinmybrain
Copy link
Contributor Author

Fixed with 3.8.5.

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

No branches or pull requests

2 participants