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/http: mention Context and Client.Do in docs for Get, Head, Post, PostForm #35562

Open
rittneje opened this issue Nov 13, 2019 · 12 comments
Open

Comments

@rittneje
Copy link

@rittneje rittneje commented Nov 13, 2019

The documentation for Client.Do says "Generally Get, Post, or PostForm will be used instead of Do." However, there is no way to pass a context.Context into those methods, so we cannot, for example, set a per-request timeout without resorting to Do. I propose adding the following new methods to http.Client.

  • GetContext(ctx context.Context, url string) (resp *Response, err error)
  • HeadContext(ctx context.Context, url string) (resp *Response, err error)
  • PostContext(ctx context.Context, url, contentType string, body io.Reader) (resp *Response, err error)
  • PostFormContext(ctx context.Context, url string, data url.Values) (resp *Response, err error)

This would effectively deprecate the existing Get, Head, Post, and PostForm methods.

We should also make a corresponding global function for each of these, and deprecate the existing http.Get, http.Head, http.Post, and http.PostForm functions.

@gopherbot gopherbot added this to the Proposal milestone Nov 13, 2019
@gopherbot gopherbot added the Proposal label Nov 13, 2019
@zikaeroh

This comment has been minimized.

Copy link
Contributor

@zikaeroh zikaeroh commented Nov 13, 2019

This doesn't resolve the proposal, but you might not be aware of ctxhttp in x/net: https://godoc.org/golang.org/x/net/context/ctxhttp

@odeke-em

This comment has been minimized.

Copy link
Member

@odeke-em odeke-em commented Nov 13, 2019

Hello @rittneje! Thank you for filing this issue. I see how this is inconvenient. However, history beat us to the punch, those functions were added years before context.Context was a thing. I am sympathetic towards adding Context on them. On the flip side though:

The highlighted functions are helper/convenience methods on the global/default HTTPClient.
To customize more things on those methods, we recommend taking a look at NewRequest et al for example https://golang.org/pkg/net/http/#Post
Screen Shot 2019-11-13 at 11 05 28 AM

The already existing customizers are:

but perhaps we should add examples enumerating how to make the various requests corresponding to those highlighted functions showing how to accomplish them but with customization and NewRequestWithContext, but also add the notice to use NewRequestWithContext for better customization.

Adding more helper functions with a new flavor would be increasing the API surface for things that we already recommend alternatives to.

/cc @bradfitz

@odeke-em odeke-em changed the title Proposal: net/http: add Context methods to http.Client proposal: net/http: add new Context suffixed helper functions for {Get, Head, Post, PostForm} Nov 13, 2019
@odeke-em odeke-em changed the title proposal: net/http: add new Context suffixed helper functions for {Get, Head, Post, PostForm} proposal: net/http: add new Context taking/suffixed helper functions for {Get, Head, Post, PostForm} Nov 13, 2019
@rittneje

This comment has been minimized.

Copy link
Author

@rittneje rittneje commented Nov 18, 2019

@odeke-em It does kind of stink that there's no real convenience method for issuing a GET request anymore without dropping the context on the floor. I don't really consider that to be a customization at all, since dropping context is a code smell. In any case, if you do decide not to add the missing helpers, you should probably remove that statement from the documentation of Client.Do since it is no longer true.

@rsc rsc added this to Incoming in Proposals Nov 27, 2019
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Dec 4, 2019

I was writing some HTTP request code earlier today and I had to use http.NewRequest and client.Do because I had to set some headers. It's not a huge deal to have to do that. The convenience functions don't have to provide for every possible convenience, or else they stop being convenient.

@rittneje

This comment has been minimized.

Copy link
Author

@rittneje rittneje commented Dec 4, 2019

@rsc I agree they don't have to provide for every possible convenience, but the point is that the old convenience methods really should never be used anymore outside toy example code. So either new versions should be added to accept a context (to not encourage developers to drop the incoming context), or the comment on Client.Do should be revised.

@alercah

This comment has been minimized.

Copy link

@alercah alercah commented Feb 11, 2020

I just was reviewing code today that was omitting contexts where it shouldn't have been, and these helper methods I imagine were a big factor in this. I would very much like to see this approved as it will save a lot of time and fix logic errors.

perhaps we should add examples enumerating how to make the various requests corresponding to those highlighted functions showing how to accomplish them but with customization and NewRequestWithContext, but also add the notice to use NewRequestWithContext for better customization.

I agree the documentation could be a little better, but I think that this characterization of contexts as a customization is wrong. In modern idiomatic Go, contexts are not customization. They're bread-and-butter mandatory parts of APIs.

the point is that the old convenience methods really should never be used anymore outside toy example code.

This hits the nail on the head. And the existing API to the package strongly implies that you want to use them, or else why are they there? It sucks to tell someone in code review "No, sorry, you can't use this convenient one-liner, you have to use a more complicated function instead just to add the mandatory context parameter."

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Feb 12, 2020

Related: see #23707 for designing a new HTTP client package.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Feb 12, 2020

I think there is plenty of short-lived command-line use for which these functions are just fine. I do agree that in a long-running program or server or a library that might be used in one of those, the helpers are not appropriate. Let's just document that more clearly. @bradfitz volunteered to write a CL.

@rsc rsc moved this from Incoming to Active in Proposals Feb 12, 2020
@rsc rsc changed the title proposal: net/http: add new Context taking/suffixed helper functions for {Get, Head, Post, PostForm} proposal: net/http: mention Context and Client.Do in docs for Get, Head, Post, PostForm Feb 26, 2020
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Feb 26, 2020

Retitled to match previous discussion about updating docs.
We can leave the more general API fixup as a separate proposal (likely based on #23707).

With the rescoping, this seems like a likely accept.

@rsc rsc moved this from Active to Likely Accept in Proposals Feb 26, 2020
@alercah

This comment has been minimized.

Copy link

@alercah alercah commented Feb 26, 2020

@cristaloleg

This comment has been minimized.

Copy link

@cristaloleg cristaloleg commented Feb 27, 2020

@alercah I suspect #23707 will happen in upcoming years, so the context will be used as it should be.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Mar 4, 2020

No change in consensus, so accepting.

@rsc rsc moved this from Likely Accept to Accepted in Proposals Mar 4, 2020
@rsc rsc changed the title proposal: net/http: mention Context and Client.Do in docs for Get, Head, Post, PostForm net/http: mention Context and Client.Do in docs for Get, Head, Post, PostForm Mar 4, 2020
@rsc rsc modified the milestones: Proposal, Backlog Mar 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Accepted
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
8 participants
You can’t perform that action at this time.