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

x/net/context/ctxhttp: replace "golang.org/x/net/context" with "context" (drop Go 1.6 and older support) #21358

Closed
dmitshur opened this issue Aug 9, 2017 · 8 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@dmitshur
Copy link
Contributor

dmitshur commented Aug 9, 2017

The golang.org/x/net/context/ctxhttp package currently has 2 different implementations for Go 1.7+ and pre-1.7. The former is much simpler, because net/http has support for contexts and cancellation built in as of Go 1.7. However both versions use golang.org/x/net/context package, not the new context in Go 1.7+ standard library.

When the Go 1.7+ support was added to ctxhttp package in CL 24620, @bradfitz wanted to avoid changing the signature and said in this comment:

adg, I didn't want to change the signature of the ctxhttp functions for compatibility reasons, so they still use golang.org/x/net/context.Context.

In a couple releases we can delete the old way.

Now that Go 1.9 final is almost out, it has been a couple releases. Is it a good time to delete the old way, and allow the package to import context instead of golang.org/x/net/context?


To avoid importing golang.org/x/net/context unnecessarily, I made a 1.7+ copy of ctxhttp at github.com/shurcooL/go/ctxhttp, but it's an extra dependency which may be problematic for people, e.g., see shurcooL/graphql#2. /cc @gmlewis

@dmitshur
Copy link
Contributor Author

dmitshur commented Aug 9, 2017

An additional thought on this. I'm not sure what the plans of golang.org/x/net/context package are. It's still not marked as deprecated. Perhaps it's being kept around for Go 1.6 and older users? Or perhaps it's going to be deleted sometime in the future.

But if something does happen to it, it might be weird to have package ctxhttp be in a subdirectory of golang.org/x/net/context.

So an alternative solution might be to copy the Go 1.7+ version of golang.org/x/net/context/ctxhttp into golang.org/x/net/ctxhttp. Then, the golang.org/x/net/context/ctxhttp package can continue to use golang.org/x/net/context indefinitely, while the new golang.org/x/net/ctxhttp package can use context right away.

@dmitshur
Copy link
Contributor Author

dmitshur commented Aug 9, 2017

Another thought. After seeing #19612 and looking at the code more closely, I can see how the 1.7+ implementation is indeed very trivial and perhaps it may no longer be needed. Meaning the package can be deprecated alongside with golang.org/x/net/context, if/when that happens.

The only part that's somewhat non-trivial and would be annoying to keep doing at every call site is this:

// If we got an error, and the context has been canceled,
// the context's error is probably more useful.
if err != nil {
	select {
	case <-ctx.Done():
		err = ctx.Err()
	default:
	}
}

But it is optional... Hmm.

@slrz
Copy link

slrz commented Aug 9, 2017

Given that x/net/context is barely more than a type alias now, there's no longer much to gain by forcing its deprecation. So, maybe just keep everything as-is for some time?

@dmitshur
Copy link
Contributor Author

dmitshur commented Oct 3, 2017

After seeing #19612 and looking at the code more closely, I can see how the 1.7+ implementation is indeed very trivial and perhaps it may no longer be needed.

@campoy pointed out that the package is still helpful if you want to do http.Get instead of http.NewRequest + req.WithContext + client.Do.

@dmitshur
Copy link
Contributor Author

dmitshur commented Nov 13, 2018

@bradfitz, we've been removing support for Go 1.6 and replacing golang.org/x/net/context with context in many packages recently (e.g., golang/net@04ba8c8). Can we make a decision of what to do (if anything) with the ctxhttp package?

I propose we just just drop Go 1.6 support, start using context from std lib. I can send a CL if that SGTU.

Discussion

My understanding is that modern ctxhttp (Go 1.7+) is nothing more than a small convenience wrapper around net/http. However, it's still useful to make passing context easier for http.Get, http.Post, etc., and for returning a slightly more useful error. It has 696 importers on godoc.org.

I see a few options:

  1. Simplest, just drop support for Go 1.6 in ctxhttp and replace golang.org/x/net/context import with context.
  2. Keep forever importing golang.org/x/net/context in golang.org/x/net/context/ctxhttp until both packages are eventually deleted (if ever), but make a copy of ctxhttp (perhaps at golang.org/x/net/ctxhttp) and have it import context. This sounds like a bad option.
  3. Keep waiting... until eventually some new API is available that makes ctxhttp no longer needed. It's unclear if/when this will happen.

I think we should go with option 1 because it's simplest.

@dmitshur dmitshur added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Nov 13, 2018
@bradfitz
Copy link
Contributor

Let's do (1).

I started work on a new HTTP client API (your option 3), btw. But I'm fine with some cleanup meanwhile.

@dmitshur
Copy link
Contributor Author

Woohoo! CL incoming.

@dmitshur dmitshur self-assigned this Nov 13, 2018
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Nov 13, 2018
@gopherbot
Copy link

Change https://golang.org/cl/149277 mentions this issue: context/ctxhttp: remove Go 1.6 support, use std context

dmitshur added a commit to shurcooL/graphql that referenced this issue Nov 14, 2018
The issue golang/go#21358 has been resolved upstream, so there's
no more need to use a temporary fork of the ctxhttp package.

Fixes #2.
dmitshur added a commit to shurcooL/issuesapp that referenced this issue Nov 14, 2018
The issue golang/go#21358 has been resolved upstream, so there's
no more need to use a temporary fork of the ctxhttp package.
dmitshur added a commit to shurcooL/notificationsapp that referenced this issue Nov 14, 2018
The issue golang/go#21358 has been resolved upstream, so there's
no more need to use a temporary fork of the ctxhttp package.
dmitshur added a commit to shurcooL/home that referenced this issue Nov 14, 2018
The issue golang/go#21358 has been resolved upstream, so there's
no more need to use a temporary fork of the ctxhttp package.

Remove the forked ctxhttp package from package listing.
It's about to be deprecated, so no point in showing it.
dmitshur added a commit to shurcooL/go that referenced this issue Nov 14, 2018
The issue golang/go#21358 has been resolved upstream, so there's
no more need to have a temporary fork of the ctxhttp package.

Also backport CL 36673 while here to have the code in sync.
dmitshur added a commit to shurcooL/play that referenced this issue Jul 18, 2019
The issue golang/go#21358 has been resolved upstream, so there's
no more need to use a temporary fork of the ctxhttp package.
@golang golang locked and limited conversation to collaborators Nov 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

4 participants