smart: implement keepalive - #5201
Open
pks-t wants to merge 3 commits into
Open
Conversation
The current stream backends do not allow for setting up any timeouts, which are required for a proper keepalive implementation. Thus, we now add a new callback `poll` to the stream backend that accepts a timeout value as parameter. The function will return as soon as there is any data available for the stream to be read or, if no data was received in the given time, it will return zero. To not break existing external implementations, `git_stream_poll` will return a positive value immediately when the backend doesn't provide a polling function. While this effectively disallows keepalive to work with such backends, this is no worse than it has been previously and we will simply block until either any data has been received in a subsequent read or until the TCP connection has been closed.
While streams now support polling for data, the transports making use of these streams don't yet. Implement a `poll` callback with the same reasoning as for the streams and provide a fallback of signalling available data immediately if no polling function has been implemented by the backend to not break existing backends. Note that the WinHTTP backend doesn't have a poll implementation right now. This is not by design, but simply because the author of this commit doesn't have any idea on how to implement it.
When receiving the packfile from a remote, git.git supports keepalive packets to not cause the connection to disconnect if the remote takes too long to start sending any bytes. Keepalive can be configured via "receive.keepAlive", which is set to 5 seconds by default. If this option is set to a positive value, then git will send a keepalive packet, which is simply an empty pkt-line "0005\1". Implement the same option for the smart protocol implementation.
Member
That's a good question. Our networking code is largely blocking and has no timeouts. I think that we should just include a |
Member
Author
|
On Mon, Aug 12, 2019 at 03:34:02AM -0700, Edward Thomson wrote:
> I think the most important questions I have is: do we want to
> use poll? Or would it be preferred to have a read_ext
> function that accepts a timeout parameter, as well?
That's a good question. Our networking code is largely
blocking and has no timeouts. I think that we should just
include a `poll` now as-is and that if we want to add timeouts
that we should look holistically at where we're at rather than
adding it in just this one place.
Fair enough, I'll polish up this PR then. Most importantly, we
should let the `poll` callback functions accept some flags like
`POLL_IN` and `POLL_OUT`, otherwise we're painting us into a
corner again.
|
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
This is a first proposal on how to implement keepalive for the smart backend as documented in git-config(1), "receive.keepAlive". It was required to change both stream as well as transport backends and extend them with a
pollfunction to support timeouts, but in fact it was done in an API-backwards-compatible way: if nopollis implemented, we simply assume that there's always data to be read and thus cannot support keepalive for such backends.Note that this is rather to be seen as an RFC for now. The
pollfunction would probably need ap_pollwrapper, which I haven't yet done. To make this clearer, I've added#include <poll.h>in the middle of the source code.I think the most important questions I have is: do we want to use
poll? Or would it be preferred to have aread_extfunction that accepts a timeout parameter, as well?Inspired by #5133.