httpclient: support googlesource #5536
Merged
Conversation
This file contains hidden or 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
Google Git (googlesource.com) behaves differently than git proper. Test that we can communicate with it.
When users call `git_http_client_read_body`, it should return 0 at the end of a message. When the `on_message_complete` callback is called, this will set `client->state` to `DONE`. In our read loop, we look for this condition and exit. Without this, when there is no data left except the end of message chunk (`0\r\n`) in the http stream, we would block by reading the three bytes off the stream but not making progress in any `on_body` callbacks. Listening to the `on_message_complete` callback allows us to stop trying to read from the socket when we've read the end of message chunk.
When `git_http_client_read_body` is invoked, it provides the size of the buffer that can be read into. This will be set as the parser context's `output_size` member. Use this as an upper limit on our reads, and ensure that we do not read more than the client requests.
The httpclient implementation keeps a `read_buf` that holds the data in the body of the response after the headers have been written. We store that data for subsequent calls to `git_http_client_read_body`. If we want to stop reading body data and send another request, we need to clear that cached data. Clear the cached body data on new requests, just like we read any outstanding data from the socket.
ethomson
changed the title
Contributor
|
Oh wow nice find! This looks to fix the Rust side of things that I was testing, thanks for this! |
|
This seems to fix the issue I tested. Thanks! |
pks-t
approved these changes
Jun 3, 2020
Member
pks-t
left a comment
pks-t
left a comment
There was a problem hiding this comment.
All of the fixes look sensible to me, thanks a lot for fixing these!
alexcrichton
added a commit
to alexcrichton/git2-rs
that referenced
this pull request
Jun 3, 2020
alexcrichton
added a commit
to rust-lang/git2-rs
that referenced
this pull request
Jun 3, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Suggestion cannot be applied right now. Please check back later.
After the great smart http refactoring of 2020, we no longer support googlesource. It turns out that google code - by default - sends very large data packets (65520) which is very neat to the upper limit of a packet size for a git data packet over the smart protocols. This illustrates some problems in our handling of very large data packets.
In particular, we read a block of data from the client and try to put it in the output buffer. When the output buffer (for data packets, this is a buffer of 65536) is nearly full, we do not take that into account and do a read of a block, despite the fact that we cannot fit it into the caller's output buffer. We then discard what cannot be sent back.
While debugging this, I found two other problems as well. This PR fixes the root cause and these other two issues.
git_http_client_read_bodywill always return0at the end of an http response. If a buffer size that is being read into is very near the number of bytes sent by the remote server, then we may be in a position where we have read all the content in the http response, and the only bytes remaining are part of the http metadata, like the zero-length chunk signifier of the end of the stream. In that case, we do not actually return content bytes, but should signify that we have finished reading the stream. Identify this by theon_message_completecallback and return0to the caller.git_http_client_read_bodytakes a buffer size that should be respected when reading from the stream. Without this, we would read data from the server, but not return it to the client, which would mean that it was lost.read_response) but the body data should be saved for a subsequent call toread_body. If such call never comes, we should clear the saved data, it should not be returned to callers for future requests.Fixes #5525