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

Jump to bottom

x/sync/errgroup: should use context.Context from stdlib #19781

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

x/sync/errgroup: should use context.Context from stdlib #19781

sbinet opened this issue Mar 30, 2017 · 13 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone
Go1.12

Comments

@sbinet
Copy link
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
Copy link
Contributor

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
Copy link

skabbes commented Sep 27, 2017

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

@ianlancetaylor
Copy link
Contributor

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

@skabbes
Copy link

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
Copy link
Contributor

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
Copy link

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
Copy link

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

@furdarius
Copy link

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)

@bradfitz bradfitz modified the milestones: Unreleased, Go1.11 Dec 17, 2017
@bradfitz bradfitz added NeedsFix The path to resolution is known, but the work has not been done. release-blocker labels Dec 17, 2017
@agnivade
Copy link
Contributor

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
@bep bep mentioned this issue Oct 2, 2018
Merged
@agnivade
Copy link
Contributor

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

@jba
Copy link
Contributor

jba commented Dec 10, 2018

AppEngine has changed. This can be fixed now.

@agnivade
Copy link
Contributor

Great, thanks !

@agnivade
Copy link
Contributor

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

@golang golang locked and limited conversation to collaborators Dec 11, 2019
shabbyrobe added a commit to shabbyrobe/golib that referenced this issue Feb 15, 2020
@shabbyrobe
Remove copypasta errgroup
5751a40 
Relevant issue has been fixed: golang/go#19781
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Milestone
Go1.12
Development

No branches or pull requests
8 participants
@bradfitz @skabbes @sbinet @agnivade @furdarius @ianlancetaylor @gopherbot @jba