WIP: Defer HTTP parsing to Hyper (h11 project) #262
Conversation
Bring Up Repo
|
Something of note, is I'm not sure whether I should anticipate supporting Python 2.7 specifically in this PR as it appears there is a discussion of its deprecation (#252) |
This comment has been minimized.
This comment has been minimized.
cheroot/server.py
Outdated
| line = self.rfile.readline() | ||
| while line: | ||
| event = self.conn.next_event() | ||
| if event is h11.NEED_DATA: |
There was a problem hiding this comment.
plz reverse this check in order to reduce the following block indentation
| if event is h11.NEED_DATA: | |
| if event is not h11.NEED_DATA: | |
| continue |
cheroot/server.py
Outdated
| if event is h11.NEED_DATA: | ||
| self.conn.receive_data(self.rfile.read(size)) | ||
| event = self.conn.next_event() | ||
| assert isinstance(event, h11.Data) |
There was a problem hiding this comment.
Is this some sort of an exit-check for ensuring contracts?
There was a problem hiding this comment.
Anything other than h11.Data here should be undefined behavior. Currently, I believe the only possible way we don't is if we get an EndOfMessage, but I'm pretty sure that were that to happen, h11 would raise an exception. I believe I saw this in the example code that they provide, so I kept it in for fun.
I can't really think of any other way it would be anything else, so I kept it in as a general catch-all.
There was a problem hiding this comment.
Alright. This is a practice people follow when doing contract programming just as a sanity-check. asserts are stripped off in optimized mode. But it's useful to keep this as an exit-check.
@ian-otto I wanted to deprecate Python 2.7 after the HTTP parser replacement so that the last py2-compatible release would get the changes and then we'd start dropping support for this. Will @njsmith thanks a lot for your comments here! |
cheroot/server.py
Outdated
| if isinstance(req_line, h11.Request): | ||
| self.started_request = True | ||
| self.uri = req_line.target | ||
| scheme, netloc, path, query, fragment = urllib.parse.urlsplit(self.uri) |
There was a problem hiding this comment.
I think you could save a few lines by doing the assignments directly
| scheme, netloc, path, query, fragment = urllib.parse.urlsplit(self.uri) | |
| _scheme, _netloc, self.path, self.qs, _fragment = urllib.parse.urlsplit(self.uri) |
cheroot/server.py
Outdated
| """The HTTPConnection object on which this request connected.""" | ||
|
|
||
| inheaders = {} | ||
| """A dict of request headers.""" |
There was a problem hiding this comment.
Should this be a dict, though? I seem to recall that there could be header duplicates...
cheroot/server.py
Outdated
| )) | ||
|
|
||
| res = h11.Response(status_code=status, headers=self.outheaders, | ||
| http_version=self.response_protocol, reason=self.status[3:]) |
There was a problem hiding this comment.
Maybe?
| http_version=self.response_protocol, reason=self.status[3:]) | |
| http_version=self.response_protocol, reason=self.status[4:]) |
|
So I've been peeking over the code reviews and I agree with most, if not all of them. I do have a few questions in regards to a few of them however. There are a few instances where I'm attempting to take things from h11 (a lot of byte-strings), and shim them into the format that the old @webknjaz Would you like me to attempt to make this change backwards compatible with the existing class, or throw that out in favor of increasing interoperability with h11? Note that some of the expectations (such as sending an HTTP 1.0 request and expecting an HTTP 1.0 response) are already going to be violated. |
Co-authored-by: Sviatoslav Sydorenko <[email protected]>
|
This pull request fixes 2 alerts when merging fa618ab into b6edd3a - view on LGTM.com fixed alerts:
|
|
This pull request fixes 2 alerts when merging b8d555a into b6edd3a - view on LGTM.com fixed alerts:
|
| # TODO: h11 doesn't have this specific of messaging. See python-hyper/h11#98 | ||
| # assert response.read(21) == b'Malformed Request-URI' |
There was a problem hiding this comment.
@ian-otto I think this can be corrected now
There was a problem hiding this comment.
Yes and no. My PR over in that repo only handled updating the messaging for a few specific cases that I felt were very important to be distinct. That said, h11 still gives fairly generic errors when it comes to request line problems. Part of me thinks that in that case, it may be okay to let that happen, since request-line issues are generally pretty easy to spot. I'm curious on your opinion though.
There was a problem hiding this comment.
I think that we could leave it as-is for now and maybe add that in the future. OTOH it'd nice UX/DX-wise to have precise messages in general.
|
As an aside, one of the tests seems to intermittently hang for seemingly days depending on CI platform. Can we set a lower timeout? |
|
@ian-otto what CI job are you talking about specifically? There's some known issues that happen flakily and are unrelated to your changes here... |
|
Back when it was still able to execute tests (running from h11's Git), I had a test timeout after 6 hours on GitHub, and another fail after 12 hours. Here are the related workflow runs: https://github.com/ian-otto/cheroot/actions/runs/189982387 Edit: I've seen the test after this fail on my local machine as well, I do think it's related to some changed behavior between the old parser and h11: |
|
@ian-otto there's no 12-hour jobs among your links. Looks like the workflows were stuck because of GitHub's outage. It should be ok now. Also, a few TLS tests are extremely flaky (mostly in slow envs like macOS or Windows) so ignore them. GH has some free minutes quota for GHA, I'm curious if those outages were factored-in... |
|
I see that you've made a few commits to this PR, but I can't pull them in. I'm not particularly sure how this works, is there a way to fetch them? P.S. The test suite that timed out on tests/3.5-ubuntu16.04. It then aborted the run, I think it did that repeatedly for every non-2.7 build. |
|
@ian-otto try Also, this branch is a bit out of date and would need to get the updates from master. Normally, I'd suggest rebasing but seeing that your branch already contains some merge commits, I'd say just merge-in master from upstream (because rebasing with merge commits is PITA most of the time). |
|
@ian-otto hey, do you need me to help you out with Git? |
| @@ -341,6 +342,7 @@ def test_malformed_request_line( | |||
| c.close() | |||
|
|
|||
|
|
|||
| @pytest.mark.xfail(reason='h11 normalizes the method and ignores non-capitalized methods') | |||
There was a problem hiding this comment.
@ian-otto looks like v0.11.0 fixes this
This comment was marked as outdated.
This comment was marked as outdated.
|
Closing as abandoned. Feel free to revive (original poster or others). |
❓ What kind of change does this PR introduce?
📋 What is the related issue number (starting with
#)Resolves #201
❓ What is the current behavior? (You can also link to an open issue here)
Currently, the Cheroot project has its own HTTP parser and logic.
❓ What is the new behavior (if this is a feature change)?
This PR will defer the primary HTTP parser to the h11 project's implementation.
📋 Other information:
Depends on: python-hyper/h11#99
📋 Checklist:
and description in grammatically correct, complete sentences
This change is