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

Comments

Projects
None yet
4 participants
@dmitshur
Member

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

This comment has been minimized.

Member

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

This comment has been minimized.

Member

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

This comment has been minimized.

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

This comment has been minimized.

Member

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

This comment has been minimized.

Member

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.

@bradfitz

This comment has been minimized.

Member

bradfitz commented Nov 13, 2018

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

This comment has been minimized.

Member

dmitshur commented Nov 13, 2018

Woohoo! CL incoming.

@dmitshur dmitshur self-assigned this Nov 13, 2018

@dmitshur dmitshur added NeedsFix and removed NeedsDecision labels Nov 13, 2018

@gopherbot

This comment has been minimized.

gopherbot commented Nov 13, 2018

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

Switch back to golang.org/x/net/context/ctxhttp package.
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

httpclient: Use golang.org/x/net/context/ctxhttp package.
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

httpclient: Use golang.org/x/net/context/ctxhttp package.
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

http: Use golang.org/x/net/context/ctxhttp package.
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

ctxhttp: Deprecate package in favor of upstream version.
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 dmitshur/go-graphql-client that referenced this issue Nov 14, 2018

transport: use golang.org/x/net/context/ctxhttp package.
The issue golang/go#21358 has been resolved upstream, so there's
no more need to use a temporary fork of the ctxhttp package.

Also, github.com/shurcooL/go/ctxhttp has been deprecated and will
be deleted, see shurcooL/go@58262d1.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment