DO NOT MERGE: debug test_unsupported_upgrade #7960
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7960 +/- ##
==========================================
- Coverage 97.53% 97.52% -0.01%
==========================================
Files 107 107
Lines 32843 32886 +43
Branches 3851 3854 +3
==========================================
+ Hits 32032 32073 +41
- Misses 610 613 +3
+ Partials 201 200 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
its getting stuck at message, payload = await protocol.read() # type: ignore[union-attr] |
if not empty_body and (
(length is not None and length > 0)
or msg.chunked
and not msg.upgrade
):
payload = StreamReader(
self.protocol,
timer=self.timer,
loop=loop,
limit=self._limit,
)
payload_parser = HttpPayloadParser(
|
aiohttp/http_parser.py
Outdated
| import pprint | ||
|
|
||
| pprint.pprint(["upgraded", msg.upgrade, msg]) | ||
| self._upgraded = code == 101 and msg.upgrade |
There was a problem hiding this comment.
c parser seems to only set this if code is 101
| @@ -505,6 +517,9 @@ def parse_headers( | |||
| close_conn = False | |||
| # https://www.rfc-editor.org/rfc/rfc9110.html#name-101-switching-protocols | |||
| elif v == "upgrade" and headers.get(hdrs.UPGRADE): | |||
There was a problem hiding this comment.
Isn't this the source of the problem for #6446? Just because the Upgrade header has content, doesn't mean there will be an upgrade. The server has every right to ignore it if the scheme is unsupported.
There was a problem hiding this comment.
If you think you can provide a fix, that'd be great. I believe there is already an xfail test from that issue, so as long as you can get that test passing..
There was a problem hiding this comment.
One problem at a time for this newbie... Brotli first 😉
| import pprint | ||
|
|
||
| pprint.pprint(["handler except", e]) | ||
| raise | ||
|
|
||
| upgrade_headers = {"Connection": "Upgrade", "Upgrade": "unsupported_proto"} |
There was a problem hiding this comment.
I'm not sure what is parsing the Upgrade header, but the fact that it's not a valid token at all should also be considered (IANA upgrade token registry. In fact, it even contains an invalid character, "_", for URL schemes (RFC 1738).
There was a problem hiding this comment.
I would assume that would be handled in the parsers (i.e. llhttp or http_parser.py, depending on whether extensions are enabled).
There was a problem hiding this comment.
Ah okay. My point is, especially it it happens externally, the test should know and account for the behavior.
I would hope, no matter which parser is used, that "unsupported_proto" would simply be thrown away and equivalent to "". Then the test should also test a valid, but unsupported, token.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| await resp.start(conn) | ||
| except BaseException: | ||
| except BaseException as ex: |
Check notice
Code scanning / CodeQL
Except block handles 'BaseException' Note
| resp.close() | ||
| raise | ||
| except BaseException: | ||
| except BaseException as ex: |
Check notice
Code scanning / CodeQL
Except block handles 'BaseException' Note
|
Looks like its an actual bug and the test only passed because of a race |
What do these changes do?
Are there changes in behavior for the user?
Related issue number
Checklist
CONTRIBUTORS.txtCHANGESfolder<issue_id>.<type>for example (588.bugfix)issue_idchange it to the pr id after creating the pr.feature: Signifying a new feature..bugfix: Signifying a bug fix..doc: Signifying a documentation improvement..removal: Signifying a deprecation or removal of public API..misc: A ticket has been closed, but it is not of interest to users.