libgit2: ensure context timeout cancels transfer #477
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
hiddeco
force-pushed
the
libgit2-timeout
branch
from
f09e14d
to
ca57aee
Compare
hiddeco
changed the title
Timeout cancels transfer
hiddeco
force-pushed
the
libgit2-timeout
branch
2 times, most recently
from
0093d58
to
0b94450
Compare
With the information from the refactor still fresh in mind, I continue to find new paths now I mentally tamed the git2go beast. `libgit2` seems to assume that a transport will eventually tell by itself that it has timed out. This also means that at present any timeout configuration does not seem have an effect. It will continue to transfer until the remote (or _something_ else) tells it is no longer transfering. This commit introduces a simple check (without tests) which was used to confirm the theory in combination with the tests in `pkg/git/strategy` (by setting it to a very low timeout and observing it fail). A future iteration should probably take the data given to the callback into account to ensure it doesn't error out if the given data[1] reports it has successfully received all objects. Another candidate for this check may be `CompletionCallback`, but one should study the C code (and likely some Go code as well) before this. In addition, to ensure the same timeout is taken into account for push operations, `PushTransferProgressCallback` may require a likewise helper. [1]: https://github.com/libgit2/git2go/blob/main/remote.go#L50-L58 Signed-off-by: Hidde Beydals <hello@hidde.co>
In transferProgressCallback(), if the received objects is equal to the total objects, return early with OK. Signed-off-by: Sunny <darkowlzz@protonmail.com>
Add SidebandProgressCallback to be able to cancel the network operation before any transfer operation. Add PushTransferProgressCallback to be able to cancel the push transfer operation. Signed-off-by: Sunny <darkowlzz@protonmail.com>
darkowlzz
force-pushed
the
libgit2-timeout
branch
from
9f10b07
to
9561ccf
Compare
|
Added
Performed some manual testing of |
hiddeco
commented
Nov 3, 2021
darkowlzz
force-pushed
the
libgit2-timeout
branch
from
cc6bfd0
to
6769485
Compare
- Adds tests for the libgit2 remote callbacks - Adds tests for CheckoutStrategyForImplementation with context timeout and verify timeout is respected by both the git implementations. Signed-off-by: Sunny <darkowlzz@protonmail.com>
hiddeco
commented
Nov 3, 2021
There was a problem hiding this comment.
Can not review my own PR, but this looks good to me. Thanks a lot for taking over and getting to the bottom of it @darkowlzz 🌻
stefanprodan
approved these changes
Nov 4, 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.
Suggestion cannot be applied right now. Please check back later.
libgit2seems to assume that a transport will eventually tell byitself that it has timed out. This also means that at present any
timeout configuration does not seem have an effect. It will continue
to transfer until the remote (or something else) tells it is no
longer transferring.
This commit adds libgit2 remote callbacks
SidebandProgressCallback,TransferProgressCallbackandPushTransferProgressCallbacktosignal libgit2 to stop the operations based on the context and the
operation status.
Refer git2go/remote.go for a list of supported remote callbacks.