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

net/context support #239

Closed
rs opened this issue Mar 1, 2016 · 11 comments
Closed

net/context support #239

rs opened this issue Mar 1, 2016 · 11 comments

Comments

@rs
Copy link

rs commented Mar 1, 2016

Services could expose a DoC(ctx context.Context) method in addition to their Do command. When a context is passed, the underlaying HTTP request would handle the context's cancellation by using ctxhttp.

@rs rs changed the title Add net/context support net/context support Mar 1, 2016
@olivere
Copy link
Owner

olivere commented Mar 1, 2016

Yes, that seems like a very good idea. Maybe we should wait a bit until the dust settles about context.Context being added to net/http in 1.7 (maybe). I'm not up to date, but I remember there was a discussion about that recently.

@rs
Copy link
Author

rs commented Mar 1, 2016

I see no sign of net/context being integrated into 1.7 unfortunately :/

@olivere
Copy link
Owner

olivere commented Mar 1, 2016

It is discussed in various places by people from the core team. But you're right: Nothing is set in stone at this point in time.

@olivere
Copy link
Owner

olivere commented Mar 2, 2016

Just two quick remarks:

  1. I expect you want to use it for cancelable requests, correct? Depending on the implementation, that's quite some work as we have many endpoints. So I think I should focus on the ones that would benefit the most. Which are the endpoints you have in mind? Search, I suppose. Anything else?
  2. If you want search timeouts, there's already a way. ES supports a timeout parameter that is exposed e.g. in the Search service and in Search source. Maybe that'll already work for you.

@rs
Copy link
Author

rs commented Mar 2, 2016

I'm using you lib in this project: https://github.com/rs/rest-layer-es

I already implemented timeout respecting context's deadline: https://github.com/rs/rest-layer-es/blob/master/util.go#L80

Now I'd like to respect cancellation as you mentioned in you point 1. The most obvious endpoints are Search, Get and MultiGet. Bulk would be my next, then Scroll and finally Update and Delete.

@olivere
Copy link
Owner

olivere commented Mar 3, 2016

I implemented the basics in Client and for the Search service in the ctxhttp branch. Will you have a look?

@rs
Copy link
Author

rs commented Mar 4, 2016

Looks great :)

@olivere
Copy link
Owner

olivere commented Mar 6, 2016

Update: Proposal to add net/context to stdlib.

olivere added a commit that referenced this issue Aug 30, 2016
Services in Elastic will now accept and honour `context.Context`. To do
that, all services now have a `Do`/`DoC` pair of methods. The latter
accepts a `context.Context`.

`Client` implements this via `PerformRequest` and `PerformRequestC`.
The latter will accept a `context.Context` as its first parameter.
If a `context.Context` is passed, Elastic uses the context-aware
`golang.org/x/net/context/ctxhttp` package to perform HTTP requests.

See e.g. #239
@olivere
Copy link
Owner

olivere commented Aug 30, 2016

It's been a looong time, but finally, in 3.0.48 you can use context.Context will all services in Elastic (see 73e440). Simply use <Service>.DoC(context.Context) instead of <Service>.Do() to pass a context.

I will publish 3.0.48 soon. It is for Elasticsearch 2.x only.

@olivere olivere closed this as completed Aug 30, 2016
@rs
Copy link
Author

rs commented Aug 30, 2016

Awesome!

@olivere
Copy link
Owner

olivere commented Aug 30, 2016

Yes. It will be for elastic.v3 only. I will only backport to elastic.v2 if it's really necessary. I'm unsure what to do about it in elastic.v5. Time will tell. ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants