Support cancellation of long-running repository operations (with context.Context)

[JoshuaSjoding]

Right now all of the Repository and Remote operations including Clone, Fetch, Pull and Push run until completion or until an error occurs. I propose adding a context.Context argument to each operation that could be long-running, so that these operations can be cancelled if the caller deems it necessary.

Here are some circumstances that such a change would allow the caller to deal with:

• A bug in go-git causes an operation to stall and not make progress
• A bug in the remote server causes it to stall and not make progress
• The remote server is malfunctioning or overburdened and is taking too long to make progress
• The network between the client and server is malfunctioning or in a poor operating condition

Adding a new argument to these operations is a breaking change, so this would be a v5 feature.

Adding a simple Timeout field to CloneOptions, FetchOptions, PullOptions and PushOptions would be insufficient. The caller may want to allow a long-running operation that is slowly making progress to continue, even though it exceeds an absolute timeout. The addition of context.Context, in combination with the existing Options.Progress, allows the caller to handle such cases.

The superficial changes to Repository are not difficult, but fully implementing context.Context throughout the plumbing and internal packages will take some effort. I propose making these changes in an iterative process with a series of pull requests, starting with the superficial changes to Repository and propagating context.Context throughout the library one step at a time.

If a context.Context is cancelled, go-git should make every effort to leave storage.Storage in a functional and uncorrupted state. I expect that this will be the most difficult part of these changes and require the most thought.

This work is a prerequisite for using go-git internally in the new dep tool.

Here's what the Remote methods would look like:

func (r *Remote) Fetch(ctx context.Context, o *FetchOptions) error
func (r *Remote) Push(ctx context.Context, o *PushOptions) (err error)

Here's what the Repository methods would look like:

func Clone(ctx context.Context, s storage.Storer, worktree billy.Filesystem, o *CloneOptions) (*Repository, error)

func (r *Repository) Fetch(ctx context.Context, o *FetchOptions) error
func (r *Repository) Pull(ctx context.Context, o *PullOptions) error
func (r *Repository) Push(ctx context.Context, o *PushOptions) error

[mcuadros]

@JoshuaSjoding great to having you back.

Here are some circumstances that such a change would allow the caller to deal with:
A bug in go-git causes an operation to stall and not make progress
A bug in the remote server causes it to stall and not make progress
The remote server is malfunctioning or overburdened and is taking too long to make progress
The network between the client and server is malfunctioning or in a poor operating condition

Make sense, is something that is very interesting.

Adding a new argument to these operations is a breaking change, so this would be a v5 feature.
We are on the v4-rc, so still we can make some changes.

The superficial changes to Repository are not difficult, but fully implementing context.Context throughout the plumbing and internal packages will take some effort. I propose making these changes in an iterative process with a series of pull requests, starting with the superficial changes to Repository and propagating context.Context throughout the library one step at a time.

Sound like a plan, are you interested on taking care of this?

If a context.Context is cancelled, go-git should make every effort to leave storage.Storage in a functional and uncorrupted state. I expect that this will be the most difficult part of these changes and require the most thought.

This is already implemented, is dependent of the storage, but works with the built-in ones.

This work is a prerequisite for using go-git internally in the new dep tool.

I opened an issue in dep a long time ago, golang/dep#424, maybe we can re-open it.

Here's what the Remote methods would look like:
func (r *Remote) Fetch(ctx context.Context, o *FetchOptions) error
func (r *Remote) Push(ctx context.Context, o *PushOptions) (err error)
Here's what the Repository methods would look like:
func Clone(ctx context.Context, s storage.Storer, worktree billy.Filesystem, o *CloneOptions) (*Repository, error)
func (r *Repository) Fetch(ctx context.Context, o *FetchOptions) error
func (r *Repository) Pull(ctx context.Context, o *PullOptions) error
func (r *Repository) Push(ctx context.Context, o *PushOptions) error

About the interface, I am not sure, maybe we can added it to the *Options structs. @smola?

[sdboyer]

Just chiming in to say that I'm happy to reopen that issue and look at this again, once the requirements @JoshuaSjoding outlined are met 😄

[JoshuaSjoding]

Great to having you back.

Thank you! This project has come a long way since I last contributed; nicely done!

Sound like a plan, are you interested on taking care of this?

Yep, I would be happy to work on a PR for this.

This is already implemented, is dependent of the storage, but works with the built-in ones.

Excellent. For storage implementations that support Transactioner we can just do a Rollback when we receive a cancellation then.

About the interface, I am not sure, maybe we can add it to the *Options structs. @smola?

I was wondering about that as well. Adding it to *Options allows us to keep the existing method signature, but adding a new ctx argument is idiomatic and generally considered best practice. My own preference is to add the ctx argument, but I'm open to direction here.

Reasons to add it to *Options:

• It's a non-breaking change that can happen in v4
• It's consistent with the way other options are supplied in these methods

Reasons to add a ctx argument:

• It's consistent with expected use of the context package.
• It's easier to make sure the context is propagated correctly when it's an argument. If it's hidden in an *Options struct it could be ignored internally in go-git without someone realizing it.
• Linters and static analysis tools will probably be happier if it's an argument.
• It forces callers to think about how they want to handle long running tasks. If they want want the old behavior it must be a conscious decision made by supplying context.Background() or context.TODO().

[smola]

The proposal looks good.

Some considerations about API and v4:

1. If this turns out to be a significant delay for v4, I wouldn't hold the release and would add it to *Options.
2. If it is only partially implemented (e.g. in Push but not in Clone), I would add it to *Options so that we don't have different approaches at the same time (as argument for methods updated before v4 and as an option for methods updated after v4).

[mcuadros]

@JoshuaSjoding I prefer not release a v5 during a long time and evolve v4 (we were making API changes during 1year) , so will be great if we can implement a very first version of the API having ctx, in all the Remote and Repository methods, where networking or long time running tasks. And then release a v4.

After release the v4 we can keep working on the implementation.

[mcuadros]

I was taking a look, and looks extremely easy add cancellation support for all the HTTP request, this looks a good starting point. @JoshuaSjoding you already started? or maybe you want me to bootstrap this?

[JoshuaSjoding]

@mcuadros I have not started yet; by all means go ahead. I won't have time to work on this until next week.

[mcuadros]

If you don't see any PR, in the next days, is that I couldn't do it. The only thing I want to do if I have time is make the first approach in order to be able to review the API.

[mcuadros]

After some tests and integration, I consider "enough" implemented to close this issue.