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

Add preliminary support for context.Context #71

Closed
wants to merge 1 commit into from

Conversation

sebnow
Copy link
Contributor

@sebnow sebnow commented Jul 20, 2017

Add support for the context concurrency pattern. This enabled control
over timeouts and deadlines when making requests to Facebook.

https://blog.golang.org/context

Add support for the context concurrency pattern. This enabled control
over timeouts and deadlines when making requests to Facebook.

    https://blog.golang.org/context
@huandu
Copy link
Owner

huandu commented Jul 26, 2017

Thanks for your PR.

I'm looking for a clean way to use Context in this library, e.g. bind a Context to a Session instance. It doesn't look good to modify lots of methods to add Context. Anyway, I haven't decided. We can discuss the design in this issue.

@sebnow
Copy link
Contributor Author

sebnow commented Jul 26, 2017

Yeah, I'm not a fan of adding context everywhere, but it makes sense. At least it's clear where the context is used.

The Context documentation suggests not to store it on structs;

Do not store Contexts inside a struct type; instead, pass a Context explicitly to each function that needs it.

This makes sense for long lived structs, such as Session might be. If a context with a timeout is passed to a Session, then requests made after the timeout will all timeout. A new Session would have to be created for each request to work around this.

Having context-aware and non-context-aware methods clutters up the API, but this was done for backwards compatibility. In the future the context-aware methods could be removed and context could be added to the current ones, removing duplication. You still get a bit of "noise" in the amount of parameters, but that's the way Go has decided to go.

I followed the convention used in the sql package (QueryContext, ExecContext, etc), so the API would be inline with stdlib. Oddly enough the http package doesn't follow this convention.

@huandu huandu closed this Oct 2, 2017
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.

2 participants