Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP types: use context.Context for public API #43

Closed
wants to merge 1 commit into from

Conversation

runcom
Copy link
Member

@runcom runcom commented Jul 18, 2016

This patch adds a context.Context argument to every public function we export in our public types.
Why?
We provide a library which makes http calls to remote services which can timeout indefinitely or more than how long the user of our library is willing to wait (or is expected to).
There's an important Go concurrency pattern which is in use in the http.Request object. Specifically the Request object provides a Cancel struct which is a channel that whenever is closed the request/response is closed accordingly. So, a user can provide his own context and do his stuff in the caller and if needed close the underlying http resource at his will w/o leaking anything.
As a plus, context.Context is being moved to the standard library in Go 1.7 and it's already widely accepted to have it as the first arg of public functions (see Docker, Protobuf, Docker/distribution.. and many more)

More info here:
https://blog.golang.org/context
https://blog.cloudflare.com/the-complete-guide-to-golang-net-http-timeouts/

Signed-off-by: Antonio Murdaca runcom@redhat.com

Signed-off-by: Antonio Murdaca <runcom@redhat.com>
@runcom runcom changed the title WIP: types: use context.Context for public API [RFC|WIP] types: use context.Context for public API Jul 18, 2016
@mtrmac
Copy link
Collaborator

mtrmac commented Jul 18, 2016

ACK on using this for HTTP-using client methods and timeout handling, allowing a single global timeout for a big complex operation is clearly the right thing to do.

Please do not use this a simple non-failing getter like DockerReference (non-failing = definitely not using a HTTP client).

I don’t know how this would interact with #41; we do need something for the auth/homedir data, and context.Context could plausibly be used with a type-safe wrapper (using the raw Context for passing untyped values around still feels icky, containers/skopeo#71 (comment) ). I guess we can do the timeout handling anyway, and then see whether it works for auth?

@runcom runcom changed the title [RFC|WIP] types: use context.Context for public API WIP types: use context.Context for public API Jul 18, 2016
@runcom
Copy link
Member Author

runcom commented Jul 19, 2016

I don’t know how this would interact with #41; we do need something for the auth/homedir data, and context.Context could plausibly be used with a type-safe wrapper

I wouldn't do this - I believe we need a type struct which carries image options like auth/capath/etc and a generic context.Context as part of http-request-methods (context.Context isn't really meant for anything else, it shouldn't be used to carry options and stuff like that - original authors stated this and I agree also)

@mtrmac
Copy link
Collaborator

mtrmac commented Jul 19, 2016

I don’t know how this would interact with #41; we do need something for the auth/homedir data, and context.Context could plausibly be used with a type-safe wrapper

I wouldn't do this - I believe we need a type struct which carries image options like auth/capath/etc and a generic context.Context as part of http-request-methods

ACK; after seeing how Context.WithValue is implemented, I wouldn’t want to put too many values in there at all.

@rhatdan
Copy link
Member

rhatdan commented Jan 29, 2018

@runcom Should we just close this PR at this point, Maybe open an issue?

@mtrmac
Copy link
Collaborator

mtrmac commented May 24, 2018

This was ultimately implemented in #431.

@mtrmac mtrmac closed this May 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants