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 and httptrace support #17

Open
delicb opened this issue Aug 30, 2016 · 6 comments
Open

net/context and httptrace support #17

delicb opened this issue Aug 30, 2016 · 6 comments
Assignees
Milestone

Comments

@delicb
Copy link

delicb commented Aug 30, 2016

First of all, great library. I really like design decisions and usability of it.

I do have some things that I miss and that I can not write plugin for. Although gentleman.context.Context is useful, I would really like to have support for net/context, especially now that it is part of standard library and net.Request has support for it.

Also, if context is added, following part should be easy, but I would also like to have some kind of support for httptrace for debugging purposes (also part of standard library since golang 1.7).

Do you have any plans of adding support for these things? And if yes, in which direction were you thinking of going?

I have experimented a bit locally and I have had some progress, but changes that I have made are not backward compatible, so I am not sure if PR would be accepted (not that I have something worthy or PR, it was just experiment).

Thanks.

@h2non
Copy link
Owner

h2non commented Sep 5, 2016

Thanks for the proposal, in fact, I'm aware about net/context, but for some design decision I've decided do not rely on it in gentleman, however, since the clear community tendency is to adopt it, especially from Go 1.7, I'm open to support or mimic natively it in a future major version where backward compatibility is not strictly required. My only concern is to keep things simple, so I would probably design an intermediate solution between the existent context and interface delegation with net/context, encapsulating it in the gentleman.Context.

As temporal solutions, I think you can write your own plugin in order to expose and encapsulate a net/context instance via the built-in gentleman's key-value context and retrieve it in other plugins.

Just one question: what are you missing from gentleman.Context where net/context would be better for you?

Feel free to fork and share your changes, I would appreciate if you can do that them in order to find potential better ideas for future implementations.

@delicb
Copy link
Author

delicb commented Sep 5, 2016

Great to hear that you are willing to consider net/context in the future.

One of the reasons why I need it, instead of gentleman.Context is ability to cancel request on external event. It might be possible to do with gentleman.Contxt, but http.Request has support for net.Context and it is already provided as part of request for services, so I would like to have ability to use same type across entire stack (from request that arrives to my service to all requests that my services sends to other services).

If I come up with anything I will make sure to send PR, so we can iterate over it to come to acceptable solution.

@h2non
Copy link
Owner

h2non commented Sep 5, 2016

Thanks for the clarification, that's was the most practical need for me as well.
Don't worry too much about the PR, if you push the code in your private fork, I can take a look, and based on that, we can discuss potential future integrations or improvements, if makes sense.

I would suggest giving it a try exposing the net/context instance via gentleman.Context shared value, as intermediate solution for now. Please note that you can also use net/context outside of gentleman's built-in privimites. Let me know if you try it.

@h2non
Copy link
Owner

h2non commented Mar 18, 2017

Go stdlib context is now supported in gentleman@v2, currently as beta preview release available in v2 branch: https://github.com/h2non/gentleman/tree/v2

@h2non h2non added this to the v2.0.0 milestone Mar 18, 2017
@h2non h2non self-assigned this Mar 18, 2017
@h2non
Copy link
Owner

h2non commented Jul 26, 2017

gentleman@v2 is finally out with first-class net/context support: https://github.com/h2non/gentleman

@ncsibra
Copy link

ncsibra commented Mar 5, 2018

Hi,

I know it's an old issue, but I think my question is related. :)
What is the correct approach to set an stdlib context to the gentleman request without losing any gentleman related data?
I have an stdlib context with a timeout and want gentleman to use that timeout, without setting explicitly through gentleman context.
My current solution is to copy the Store to my context and set that to the http.Request, something like:

ctx, _ = context.WithTimeout(context.Background(), 5 * time.Second)
ctx = context.WithValue(ctx, gcontext.Key, req.Context.GetAll())
req.Context.Request = req.Context.Request.WithContext(ctx)

Is it ok or am I missed something in the documentation?

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

3 participants