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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Implement h2 protocol #1026

Closed
wants to merge 50 commits into from
Closed

Conversation

Vibhu-Agarwal
Copy link
Contributor

@Vibhu-Agarwal Vibhu-Agarwal commented Apr 30, 2021

Closes #47

HTTP Parsing

  • Using h2

PR #214

It was great! I've picked a lot of stuff from there.
I've mostly worked on extending the PR, removing bugs and modernizing the code.
I've opted to omit a few implementations though, on which I'd like to have some discussion.

Motivation from Hypercorn's code

I also read the hypercorn's code, which has the implementation for HTTP/2 already. I got a pretty good idea of http2's working from there.

priority

It uses priority to build a priority tree and manage the streams.
I haven't yet implemented this in this PR (as it wasn't, in #214). Should we use it?
If we use it, it will also help in implementing code for handling h2 events like WindowUpdated, PriorityUpdated and RemoteSettingsChanged (which are currently ignored).

stream_buffers

Hypercorn also uses StreamBuffers to handle the data of different streams per connection. This may be due to the fact that h2 doesn't buffer data internally for sending. I'm not sure though, as hypercorn checks the chunk size (before sending) anyway.
I haven't yet implemented this in this PR (as it wasn't, in #214). Should we use it?

Server Push

I wasn't sure if I should go ahead with HTTP Server-Push's implementation or not, as its status is not clear.
Should we support it?

In this PR

Tests

  • For tests for config.py,
    • It didn't have much to cover up the coverage, but SSL-things had to be tested. So, one for that.
  • For tests for h11_impl.py,
    • These two upgrades need tests:
      1. h2c-upgrade
      2. H2-Prior-Knowledge Connection-Preface
    • Work: I added one for both of them (got the coverage; but checks in response still remain):
      1. h2c-upgrade: I added one for a simple GET request.
        For some reason, test_h2c_upgrade_request is causing failure of some un-related tests in Python3.8 and Python3.9 (runs fine in Python <=3.7). Commit 81f21be
        Manual-Testing: I'm not sure how to manually test this one.
      2. H2-Prior-Knowledge Connection-Preface: The test is really a minimal one, not even hitting any endpoint. So I'm not even sure if this is sufficient or not.
        Manual-Testing: I tried this with a POST request using curl. It works. I'm not sure how to replicate it in Python or even get raw HTTP request out of it.
    • Basically, I'm out of ideas to implement tests for these.
  • For tests for h2_impl.py,
    • Just like HTTPToolsProtocol and H11Protocol, H2Protocol needs testing for a lot of stuff.
    • I saw two options of implementation methods:
      1. test_http.py way (using mocks). I tried this out (had to modify get_connected_protocol() a bit) but got messed up with valid raw HTTP/2 requests and SSL-Context passing. Couldn't debug and get it to work.
      2. test_ssl.py way (using httpx). I installed Starlette, added a (very) small app, and tried out a test with a simple POST request. It kinda worked out. So I've added this one in the PR.
    • Manual-Testing: I tried with my (Firefox) browser using a sane app on simple GET requests. It seems to work fine.
      I haven't tried broken apps and others with exceptions.

WIP

  • I've marked a lot of places in the code with # TODO: Task and added some comments for decision-related discussion or missing implementation.
  • I've also tried to keep the code of h2_impl.py similar to httptools_impl.py and h11_impl.py.

Help Needed

I'm just giving it a go. Hope to complete this soon 馃
Will need reviews, inputs, tests, ideas and a lot of help from everyone 馃

@Vibhu-Agarwal Vibhu-Agarwal marked this pull request as ready for review June 1, 2021 23:26
@Vibhu-Agarwal
Copy link
Contributor Author

Vibhu-Agarwal commented Jun 1, 2021

^ So that some discussion could be started.
The Work is still In-Progress though.

@@ -20,6 +20,7 @@ types-pyyaml
trustme
cryptography
coverage
starlette
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need starlette? What's the idea here?

@@ -47,6 +47,7 @@ def get_packages(package):
"asgiref>=3.3.4",
"click>=7.*",
"h11>=0.8",
"h2>=4.0.0",
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines +555 to +556
# TODO: Implement or Not?
# https://groups.google.com/a/chromium.org/g/blink-dev/c/K3rYLvmQUBY/m/vOWBKZGoAQAJ 馃槙 # noqa: E501
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should, yep.

@tomchristie
Copy link
Member

So, I'm neither necessarily for or against this, but I do have an alternate perspective that I think is worth sharing/considering.

See the discussion in... #1105

@tomchristie
Copy link
Member

It may well make sense for us to deliberately limit the scope of Uvicorn to being an HTTP/1.1 server.
The tighter the scope, the more focused our resources can be on making sure we do a fantastic job in a particular space.

@Kludex
Copy link
Sponsor Member

Kludex commented Jan 6, 2022

@Vibhu-Agarwal Thanks for the effort here, but I think by #1105 we'll highlight hypercorn as the recommendation for HTTP/2.

Sorry for taking so long to act here.

@Kludex Kludex closed this Jan 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HTTP 2.0 Support
3 participants