Repository navigation
Add context propagation to rclone #3257
Description
Activity
Add context propagation to rclone
This issue is a proposal to refactor rclone by adopting
context.Contextpropagation in internal usage.rclonewas created when this was not a common practice so adopting it would open possibilities for new features, allow it to play better with other Go projects and clear up some technical debt.Agreed and thank you very much for stepping up to do this work.
Intention is to make only internal structural changes so everything that works and is added today must continue to work once contexts are adopted.
Adoption of contexts will add two main benefits:
- Ability to propagate cancellation signals, so transfers can be canceled before they are finished
- Ability to propagate request scoped values through call chains without creating coupling between abstractions
Agreed.
Proof of concept
I've created a fork where big part of this proposal is already implemented (almost all of it). Actual commit where context propagation is addressed can be found here ajankovic@8c7b876
It's a big change for a single commit but it was necessary to keep changes together to avoid breaking the build.
My approach was to first change the base interfaces in
fspackage and move on from there.I usedAWS s3backend tests as base for deciding to which interface call I should add context. Context spread like virus so it ended up even in some places I haven't initially expected it to go.After I was satisfied on changes to the interfaces I followed compiler complaints by running
make quicktestand making changes until it stopped complaining.Current state of the POC is that it's working for the use case that I needed it for. I was able to implement Job cancelation and grouping of transferred files by the Job based on it. But it needs review and some design considerations before it can be merged upstream.
:-)
Challenge 1: How to refactor
fsinterfaces?End result can be seen here ajankovic@8c7b876#diff-f6c6bb8a6ec40b9f63d107bc7070f708
You can use it to follow my explanations for each interface:
fs.Info
I've decided against adding context to
fs.Infoas it's a interface based around static data.I think this is fine.
fs.Fs
Every call needs a context because there is a great possibility that it does interaction with remote service.
Agreed
fs.Object
Every call needs a context because there is a great possibility that it does interaction with remote service.
Agreed.
fs.ObjectInfo
In my opinion only
ObjectInfo.Hashneeds context here as it might need to fetch object or request hash from remote.I wasn't sure so sure about
ObjectInfo.Storablebecause I wasn't sure what it's actual purpose is. I decided against adding context to it. There are some cases where I found that backend needed to do remote call because of it but I think it was the good tradeoff to not add it.Hash/Modtime/MimeType can fetch stuff from the remote but usually won't be.
Storable() is only really used by the local backend. I think it could probably be got rid of actually!
fs.DirEntry
This one was tricky. I decided to only add context to
DirEntry.ModTimeas it was needed byAWS s3. Later I've seen that some backends need context forDirEntry.Sizebut I decided against using it there.I am wondering shouldn't a good practice by backends be to load this information when object is created and serve it statically? Maybe that's not always possible?
Some remotes (like s3) store ModTime as metadata which isn't returned in the object listing, so an extra read is needed to fetch it.
Size() I would have thought should be a constant and stored in the object for just about every backend.
fs.MimeTyper
AWS s3was using it to read metadata so I added it.OK
fs.Features and others
The optional methods in the Features structs all do stuff - I think you got that right.
I followed the same logic, if there is an expected blocking interaction then add context to it. Please take a look at the POC and voice your concerns.
I looked at your changes in
fs/fs.goand I think they are good :-)Challenge 2: What to do with tests?
If we take initial constraint of making sure that everything must work like it did without contexts, then adding contexts to tests shouldn't change anything. That means we should just use
context.Background()in every place were context is required. That's what I did so all api changes that have existing tests are satisfied withcontext.Background().Great
Challenge 3: How to update backends?
Again by the initial constraints backends are only updated by making sure they satisfy modified interfaces. Assumption is that after update, contexts will be available in the function call so it is optional to use it but shouldn't brake existing functionality. Context can be propagated gradually in some later releases as the maintainers see fit.
One special case is
AWS s3backend where propagating was easy (client provides *WithContext replacements) and it was relevant for my particular use case.Also I've noticed that some backends use clients that don't even have option for propagating context with the request and this will require some additional work later on. So documentation needs to be maintained with information which backend supports this and which does not.
Some of the backends should be easy to update. Anything using
lib/restcan be adapted quite easily (with a small patch tolib/rest).I think we should change the interfaces for everything first, then update the backends in separate commits just to try to reduce the impact of the first commit.
Challenge 4: What to do with
NewFs?It was obvious to me that some remote access initialization is needed for most backends but I figured out this is only needed once and there is no need for actually controlling this process because you need Fs object created. So I just opted for
ctx := context.Background()pattern at the top of theNewFswhere all calls with context in init will use this context. Maybe this needs change it worked for me.NewFs is called from the API too. We should probably add context to it at some point because of that.
All other places that I didn't know what context to pass I've used
context.TODO().Great.
Challenge 5: How to propagate context to other mechanisms?
All top level functions needed context because it must be propagated to the lower levels. I've also changed
rc.Functo accept context. There are some dubious changes tovfspackage.I'll stop here and ask for your review of the changes that are implemented by the ajankovic@8c7b876 commit.
I'll run through all the changes in the commit and put notes inline if I see anything.
In conclusion
I've tried to make it as generic as possible for the broader rclone usage but I was mainly governed by my own use cases. Please take time to look at it and we can discuss what needs to be done to make it work for the upstream as well.
Great job :-)
My aim with this patch is we can get to a point where we can rebase it onto master with no conflicts. This may require a bit of work on your part keeping up with changes in the repo so let's try to keep the momentum going!
We are in the run-up to a release for v1.48 (at the weekend) so I won't want to merge stuff before then.
I had a look through the code.
Most of the changes are somewhat mechanical so I skimmed over those.
I saw the VFS changes. I think that will require more work
- the fuse library rclone uses passes contexts for the calls which we are currently ignoring
https://github.com/ncw/rclone/blob/939b19c3b723fe6c9dd0fd56765dd5b0e404c9ba/cmd/mount/file.go#L26 - we need not to break the API as there are various parts of rclone (eg the webdav library) which are expecting a standard Go file system API
https://github.com/ncw/rclone/blob/939b19c3b723fe6c9dd0fd56765dd5b0e404c9ba/cmd/serve/webdav/webdav.go#L114
And a thought... Do we need to context-ify methods that don't return an error? If they don't return an error then we can't return the "context cancelled" error?
So here is what I think we should do (starting after the v1.48 release)
- get your initial patch merged pretty much as-is
- I'd like to run the integration tests against it at some point
- make a second patch for NewFs
- looking at the code I think it makes sense to do that too
- patch lib/rest to take a ctx in
Calland friends
https://github.com/ncw/rclone/blob/939b19c3b723fe6c9dd0fd56765dd5b0e404c9ba/lib/rest/rest.go#L184 - make individual patches to context-ify backends
- think about the vfs layer some more!
- the fuse library rclone uses passes contexts for the calls which we are currently ignoring
I've created a PR for this change as per your instructions. We should probably merge it before development for v1.49 speeds up.
I've merged that now!
Next on the list
- make a second patch for NewFs
- looking at the code I think it makes sense to do that too
- patch lib/rest to take a ctx in
Calland friends
https://github.com/ncw/rclone/blob/939b19c3b723fe6c9dd0fd56765dd5b0e404c9ba/lib/rest/rest.go#L184- Will be simple to integrate using Request.WithContext
- this will need patches to the lib/rest using backends too, but should finish contextifying them.
- make individual patches to context-ify other backends
- you did this for s3, it is probably possible for more of the other backends but I haven't investigated in detail
- think about the vfs layer some more!
Would you like to work on some of these?
BTW I have an unmerged backend for google photos and it took about 5 minutes to get it compiling again, using compiler error message debugging!
- make a second patch for NewFs
Yes I could work on first two, and would need help for from you for the last two. Maybe I can also make modifications to the backends that are using clients from the second. The fourth should be done by you as I am overloaded at the moment to understand implications fully.
Yes I could work on first two, and would need help for from you for the last two. Maybe I can also make modifications to the backends that are using clients from the second. The fourth should be done by you as I am overloaded at the moment to understand implications fully.
That would be great. The first two should be mostly mechanical changes.
I'm happy to do the third and fourth :-)
- added a commit that references this issue
on Jun 19, 2019 - added 2 commits that reference this issue
on Jun 19, 2019 43 remaining items
- added 15 commits that reference this issue
on Jul 20, 2026
Add context propagation to rclone
This issue is a proposal to refactor rclone by adopting
context.Contextpropagation in internal usage.rclonewas created when this was not a common practice so adopting it would open possibilities for new features, allow it to play better with other Go projects and clear up some technical debt.Intention is to make only internal structural changes so everything that works and is added today must continue to work once contexts are adopted.
Adoption of contexts will add two main benefits:
Proof of concept
I've created a fork where big part of this proposal is already implemented (almost all of it). Actual commit where context propagation is addressed can be found here ajankovic@8c7b876
It's a big change for a single commit but it was necessary to keep changes together to avoid breaking the build.
My approach was to first change the base interfaces in
fspackage and move on from there.I usedAWS s3backend tests as base for deciding to which interface call I should add context. Context spread like virus so it ended up even in some places I haven't initially expected it to go.After I was satisfied on changes to the interfaces I followed compiler complaints by running
make quicktestand making changes until it stopped complaining.Current state of the POC is that it's working for the use case that I needed it for. I was able to implement Job cancelation and grouping of transferred files by the Job based on it. But it needs review and some design considerations before it can be merged upstream.
Challenge 1: How to refactor
fsinterfaces?End result can be seen here ajankovic@8c7b876#diff-f6c6bb8a6ec40b9f63d107bc7070f708
You can use it to follow my explanations for each interface:
fs.Info
I've decided against adding context to
fs.Infoas it's a interface based around static data.fs.Fs
Every call needs a context because there is a great possibility that it does interaction with remote service.
fs.Object
Every call needs a context because there is a great possibility that it does interaction with remote service.
fs.ObjectInfo
In my opinion only
ObjectInfo.Hashneeds context here as it might need to fetch object or request hash from remote.I wasn't sure so sure about
ObjectInfo.Storablebecause I wasn't sure what it's actual purpose is. I decided against adding context to it. There are some cases where I found that backend needed to do remote call because of it but I think it was the good tradeoff to not add it.fs.DirEntry
This one was tricky. I decided to only add context to
DirEntry.ModTimeas it was needed byAWS s3. Later I've seen that some backends need context forDirEntry.Sizebut I decided against using it there.I am wondering shouldn't a good practice by backends be to load this information when object is created and serve it statically? Maybe that's not always possible?
fs.MimeTyper
AWS s3was using it to read metadata so I added it.fs.Features and others
I followed the same logic, if there is an expected blocking interaction then add context to it. Please take a look at the POC and voice your concerns.
Challenge 2: What to do with tests?
If we take initial constraint of making sure that everything must work like it did without contexts, then adding contexts to tests shouldn't change anything. That means we should just use
context.Background()in every place were context is required. That's what I did so all api changes that have existing tests are satisfied withcontext.Background().Challenge 3: How to update backends?
Again by the initial constraints backends are only updated by making sure they satisfy modified interfaces. Assumption is that after update, contexts will be available in the function call so it is optional to use it but shouldn't brake existing functionality. Context can be propagated gradually in some later releases as the maintainers see fit.
One special case is
AWS s3backend where propagating was easy (client provides *WithContext replacements) and it was relevant for my particular use case.Also I've noticed that some backends use clients that don't even have option for propagating context with the request and this will require some additional work later on. So documentation needs to be maintained with information which backend supports this and which does not.
Challenge 4: What to do with
NewFs?It was obvious to me that some remote access initialization is needed for most backends but I figured out this is only needed once and there is no need for actually controlling this process because you need Fs object created. So I just opted for
ctx := context.Background()pattern at the top of theNewFswhere all calls with context in init will use this context. Maybe this needs change it worked for me.All other places that I didn't know what context to pass I've used
context.TODO().Challenge 5: How to propagate context to other mechanisms?
All top level functions needed context because it must be propagated to the lower levels. I've also changed
rc.Functo accept context. There are some dubious changes tovfspackage.I'll stop here and ask for your review of the changes that are implemented by the ajankovic@8c7b876 commit.
In conclusion
I've tried to make it as generic as possible for the broader rclone usage but I was mainly governed by my own use cases. Please take time to look at it and we can discuss what needs to be done to make it work for the upstream as well.
Backends done