Implement experimental HTTP/2 support #4769
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
BREAKING CHANGES - Request is sent successfully with its Response received as well. However, the StreamEnded event is not received which do not fires the response deferred
- Remove all wrapper funtions made such that stream can send header/data to H2Connection as they were not necessary BREAKING CHANGES Looks like, for small set of response data the StreamEnded event is emitted and everything works well -- tested for both GET & POST request. Maybe some issue with window size and/or flow control as when the response data needs to be broken into separate chunks -- not all chunks are received everytime which leads to indefinite waiting for next data chunk and the connection is lost due to timeout.😥 Working on setting up testing environment now. After testing is setup I'll debug the above bug furthur.
Every data chunk received needs to be acknowledged to - update the flow control window size - get furthur data chunks from the server
- make separate function to parse http headers from Request instance
- Use itertools.count to generate next stream_id BREAKING CHANGES When sending data/body more than the local flow control window -- no window update occurs to send the remaining data frames. Hence, the complete body is not send resulting in no response received.
- add Twisted[http2] in setup.py requirements - add test_protocol.py to test the current implementation BREAKING CHANGES test_download times out because of no protocol negotiated between Mockserver and HTTP/2 client
- Add StreamCloseReason enum - Send response for different cases considering download_warnsize, download_maxsize, fail_on_data_loss, connection lost, etc.
- remove test_protocol.py as working testing environment is setup🙂 🙃 - Add typing_extensions as dependency to support TypedDict for python<3.8
- Remove repeated dependency Twisted from setup.py - Test for both GET & POST when - Only 1 request - Large number (=20) of requests and - Small Data (10 KB) per request - Large Data (10 MB) per request - Test when request is cancelled by the client' BREAKING CHANGES Tests raises OpenSSL.SSL.Error when run using tox. However, all tests passes when ran using `python -m unittest`.
- rename LOGGER -> logger - remove self._write_to_transport from Stream class and handle all transport related activities inside HTTP2ClientProtocol class
- add required test -- test by sending 1000 requests - increase test timeout to 180 seconds to account for tests taking long time
- while testing the job exceeded the maximum log length and was terminated - reduce the number of requests from 20 to 10
- refactor from str.format() to f-strings
- Initiating requests having hostname or (ip_address, port) different from the peer to which HTTP/2 connection is made can lead to closing the whole connection and close out all the pending streams. - This change aims to fix that problem - Add required tests - Save hostname & port in H2ConnectionMetadataDict
- Change log level for hpack to ERROR
- H2ClientProtocol.close_stream - Fix and add missing type hints - More adjustments - Rename stream id generator - Simplify decrement
negotiatedProtocol's type is Optional[bytes] See https://github.com/twisted/twisted/blob/twisted-20.3.0/src/twisted/protocols/tls.py#L563-L587 and https://www.pyopenssl.org/en/20.0.1/api/ssl.html#OpenSSL.SSL.Connection.get_alpn_proto_negotiated Note that OpenSSL.SSL.Connection.get_next_proto_negotiated is deprecated: https://www.pyopenssl.org/en/20.0.0/changelog.html#backward-incompatible-changes
|
@Gallaecio Regarding the memory issues, I'd like to point out a similarity with the |
|
@elacuesta I ended up just re-running the tests. I know it’s not a fix, but since the issue is not specific to any change here (it’s most likely reproducible in master, given it affects a documentation pull request like #4974), this is probably not the right pull request to fix it. |
|
Oh I didn't realize other PRs were affected as well, much less one about docs. In that case, it's no reason to hold this one from merging. |
elacuesta
reviewed
Feb 24, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Resolves #1854
This is most of the GSoC 2020 work by @adityaa30. There is additional work which will be handled in separate pull requests.
To do:
Request.protocolsupport (Response.protocol attribute #4878) (done in e494a3f)bytes_receivedsignal is not implementedAfter merging:
bytes_receivedsignaldatalossflag as in the HTTP/1.1 response handler