Limit protocol upgrades to supported protocols

[lphuberdeau]

What do these changes do?

Fix issue #2277 by limiting connection upgrades to supported protocols. Currently only websocket as h2 and h2c are not supported.

There are no unit tests on this change as all my attempts caused the test to be skipped. Could use some help.

Are there changes in behavior for the user?

Responses will be obtained when the server advertises connection upgrades to http2 without the client requesting it.

Related issue number

#2277

Checklist

• I think the code is well written
• Unit tests for the changes exist
• Documentation reflects the changes
• If you provide code modification, please add yourself to CONTRIBUTORS.txt
  • The format is <Name> <Surname>.
  • Please keep alphabetical order, the file is sorted by names.
• Add a new news fragment into the CHANGES folder
  • name it <issue_id>.<type> for example (588.bug)
  • if you don't have an issue_id change it to the pr id after creating the pr
  • ensure type is one of the following:
    • .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.
  • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

[asvetlov]

No, aiohttp supports upgrade to custom protocol IMHO.
@fafhrd91 please correct if I'm wrong.
@lphuberdeau what is your use case?

[fafhrd91]

I see one main reason is http/2, if server can not upgrade to http/2 it should ignore upgrade header and serve request as a http/1 request.
But for this use case custom logic should be implemented

[fafhrd91]

Though I have plans to add http/2 support to aiohttp

[lphuberdeau]

@asvetlov I have the same issue as reported. aiohttp provides blank responses on some sites and it's because of this apparently. #2277

It seems to be related to mod_h2 in apache where the server always responds with Upgrade: h2 even if the client did not request it. Browsers and all other tools we've tested give the response just fine. aiohttp hangs for a bit and errors out based on stream not being readable.

Commenting out this line fixes the issue (although it's not a real fix)
https://github.com/aio-libs/aiohttp/blob/master/aiohttp/client_proto.py#L173

This condition is also part of the problem, although I'm not sure I understand it.
https://github.com/aio-libs/aiohttp/blob/master/aiohttp/http_parser.py#L166

I'm fully aware the proposed fix is missing out on a lot of context ;) Glad to have this conversation.

[lphuberdeau]

@fafhrd91 The issue here is really that the server includes the Upgrade: h2 header in the response even if the client does not request it. We actually found several cases of this, all of which were under Apache so far.

Chunked response also, which kind of points towards that http_parser.py#L166 line.

HTTP2 support would be great!

[fafhrd91]

“upgrade: h2” is standard http/2 upgrade process for clear connection upgrade. Spec states that server should respond with http/1 if it does not understand http/2. So this is actually aiohttp bug

[lphuberdeau]

The patch could easily be reversed to use a black list on connection upgrade to h2 instead and still resolve the issue if the whitelist is too restrictive. It could possibly also be resolved higher in the stack.

Let me know how I can help.

[asvetlov]

Fixed on master.
Please feel free to create a new issue if problem will back again.

[lphuberdeau]

Thanks for taking care of this. For educational purposes, could you identify which commit fixes the issue?

It would also help track which release the fix will be in.

[asvetlov]

Sorry, I've missed concrete commit hash number.
Just this script https://gist.github.com/Findus23/3aaedbac0dd2520a9b65d0c9a6431b22 works on master but fails on aiohttp 2.3

The master will be published as aiohttp 3.0

[yelite]

@lphuberdeau I think it's 79655eb. You can also check the commit in upstream, nodejs/http-parser@05525c5

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a [new issue] for related bugs.
If you feel like there's important points made in this discussion, please include those exceprts into that [new issue].
[new issue]: https://github.com/aio-libs/aiohttp/issues/new