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/sync/errgroup: should use context.Context from stdlib #19781

Closed
sbinet opened this Issue Mar 30, 2017 · 13 comments

Comments

Projects
None yet
8 participants
@sbinet
Member

sbinet commented Mar 30, 2017

The x/sync/errgroup/errgroup.go file reads:

// Package errgroup provides synchronization, error propagation, and Context
// cancelation for groups of goroutines working on subtasks of a common task.
package errgroup

import (
	"sync"

	"golang.org/x/net/context"
)

// [...]

// WithContext returns a new Group and an associated Context derived from ctx.
//
// The derived Context is canceled the first time a function passed to Go
// returns a non-nil error or the first time Wait returns, whichever occurs
// first.
func WithContext(ctx context.Context) (*Group, context.Context) {
	ctx, cancel := context.WithCancel(ctx)
	return &Group{cancel: cancel}, ctx
}

it should probably import and use "context" instead of "golang.org/x/net/context" (once AppEngine jumps to Go1.8)

@gopherbot gopherbot added this to the Unreleased milestone Mar 30, 2017

@bradfitz

This comment has been minimized.

Member

bradfitz commented Mar 30, 2017

We'll do this when Go 1.9 is released. (We generally try to support the past few Go releases in the golang.org/x/* repos)

@skabbes

This comment has been minimized.

skabbes commented Sep 27, 2017

I ran into this today on Go 1.9, any chance to get this fixed?

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Sep 27, 2017

Yes, I think we can make this change now. Want to send a patch?

@skabbes

This comment has been minimized.

skabbes commented Sep 27, 2017

What are the compatibility requirements, should I use conditional compilation? or can I simply just use "context". Additionally, where is the repository for this? I suppose I need to set up gerrit as a go contributor, but is the x/sync packaged hosted somewhere else?

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Sep 27, 2017

I think we can just use "context". At least, we can see if anyone complains.

You can fetch the package via go get golang.org/x/sync. That will give you a git repo that should work with Gerrit.

@skabbes

This comment has been minimized.

skabbes commented Sep 27, 2017

First go commit, I think I did everything correctly. Changeset here:

https://go-review.googlesource.com/c/sync/+/66651

@gopherbot

This comment has been minimized.

gopherbot commented Dec 17, 2017

Change https://golang.org/cl/84481 mentions this issue: x/sync/errgroup, x/sync/semaphore: use std context instead of x/net/context

@furdarius

This comment has been minimized.

furdarius commented Dec 17, 2017

https://golang.org/cl/84481 replace context without complexity. So, it can be merged when App Engine will move to Go1.8 ("the policy they decided upon is to wait until January 2nd" - by Joe Tsai)

@agnivade

This comment has been minimized.

Member

agnivade commented Apr 1, 2018

Update on the issue from the CL thread. If anyone wants to follow -

the App Engine team at Google has decided that Go 1.6 users need to be supported for longer, so the decision was made to continue to use golang.org/x/net/context for some time in all the golang.org/x/* (non-Google) repos. We don't have test coverage of them at https://build.golang.org/, but we're relying on them being tested via other testing dashboards.

It'd be nice if the App Engine team made an official statement somewhere about the policy Go now needs to follow, even if it's not tested.

@bradfitz bradfitz modified the milestones: Go1.11, Go1.12 Jun 20, 2018

@agnivade

This comment has been minimized.

Member

agnivade commented Dec 10, 2018

@jba - Should this be pushed to 1.13 ? Doesn't look like there has been any change to AppEngine policy.

@jba

This comment has been minimized.

jba commented Dec 10, 2018

AppEngine has changed. This can be fixed now.

@agnivade

This comment has been minimized.

Member

agnivade commented Dec 10, 2018

Great, thanks !

@agnivade

This comment has been minimized.

Member

agnivade commented Dec 11, 2018

This is done now. The CL was just marked as merged as the change was already in effect.

@agnivade agnivade closed this Dec 11, 2018

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