Add inspectable context parameter to API calls

[zebwaredano]

I am currently working on a product in which multiple goroutines are sharing the same swift client, and we need to be able to correlate the requests the client is making with the specific goroutine which initiated the action. To do this we pass down the request ID and make it accessible to a custom http.Transport for logging. We do this successfully with other storage clients. However, with this Swift client we've had to do some hacking. We append request ID to the Connection.UserAgent field, then extracting in our overriden http.Transport.RoundTrip method before dispatching the request.

This seemed to us was a generic enough use-case to open an issue. We suggest Google's approach of adding a context parameter to each API function call, https://godoc.org/cloud.google.com/go/storage I understand this would mean updating the API functions to accept a context AND attach the context to the requests the client makes to allow a custom Transport to inspect the request's context. Not only would it support our use case, but make it far easier for your users to process custom data in their net/http interface implementations.

[ncw]

The swift library was started long before Google invented the context library round about go1.0!

Looking at now it is sorely lacking context! I've felt the lack when using this library in rclone.

I don't wish to break the public API in v1 so I guess the choices are

1. Add new methods which take a context parameter so ObjectCtx, ObjectCopyCtx etc. There are 55 public methods on Connection :-(
2. go for v2 of the library - go team's blog post on how to do that
3. Do a hack such as this

c.WithContext(ctx).Object()

Option 1 is relatively straight forward - rename all existing methods with ...Ctx(ctx context.Context... propagate the context, shim the old methods in.

Option 2 requires the v2 go module stuff which I'm not a fan of and is probably a similar amount of work. That would leave maintaining two versions of the library too...

Option3 would require renaming all the methods to be internal then shimming them to retain compatibility then wrapping them again for the object returned by WithContext so that is the most work probably.

So Option 1 probably has it. What do you think? Would you be willing to help with this?

[zebwaredano]

Sure! I've got a bit much on my plate at work atm, but would like to help as much as I'm able. Option 2 seems a bit overkill and as you mentioned maintaining multiple versions can be painful. Option 3 would mean the user would've to keep track of 2 different connection instances if they want to make calls with and without context. Option 1 seems to me the best as well.

So the context would be propagated down to a new (c *Connection) CallCtx which would then use http.NewRequestWithContext(propagatedCtx, ...)? The internal functions such as (c *Connection) storage(p RequestOpts) could be made to take an additional context parameter, and the old, ctx-less methods just pass down a context.Background()?

[ncw]

Sure! I've got a bit much on my plate at work atm, but would like to help as much as I'm able.

Great!

Option 2 seems a bit overkill and as you mentioned maintaining multiple versions can be painful. Option 3 would mean the user would've to keep track of 2 different connection instances if they want to make calls with and without context. Option 1 seems to me the best as well.

OK!

So the context would be propagated down to a new (c *Connection) CallCtx which would then use http.NewRequestWithContext(propagatedCtx, ...)?

Yes that sounds perfect.

There are some other places that could do with contexts like the timeouts and things, but they can come later.

The internal functions such as (c *Connection) storage(p RequestOpts) could be made to take an additional context parameter, and the old, ctx-less methods just pass down a context.Background()?

That is what I was thinking, yes.

This will be mostly an exercise in passing the context about!

I'd probably do this by adding ctx context.Context to Call then following the compiler warnings! After that was done then renaming the methods that have a ctx to ...Ctx() and shimming in the old one to call that with context.Background is a pretty mechanical job.

[FUSAKLA]

Hi, I found a free moment and took a stab at this, PTAL @ncw @zebwaredano #162

[ncw]

This is merged now - thank you very much @FUSAKLA - much appreciated :-)

[FUSAKLA]

🎉 I'll update to v2 in the Thanos project right away :)

[ncw]

Rclone passed the integration tests with v2 :-)

I'm not going to merge it until v1.54 though since it doesn't work with go1.12 and I need to deprecate that first!