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

proposal: context: expose canceler interface #28728

Open
gobwas opened this Issue Nov 11, 2018 · 5 comments

Comments

Projects
None yet
4 participants
@gobwas
Copy link

gobwas commented Nov 11, 2018

Hello there,

This proposal concerns a way to implement custom context.Context type, that is clever enough to cancel its children contexts during cancelation.

Suppose I have few goroutines which executes some tasks periodically. I need to provide two things to the tasks – first is the goroutine ID and second is a context-similar cancelation mechanism:

type WorkerContext struct {
    ID uint
    done chan struct{}
}

// WorkerContext implements context.Context.

func worker(id uint, tasks <-chan func(*WorkerContext)) {
    done := make(chan struct{})
    defer close(done)

    ctx := WorkerContext{
        ID: id,
        done: make(chan struct{}),
    }

    for task := range w.tasks {
        task(&ctx)
    }
}

go worker(1, tasks)
go worker(N, tasks)

Then, on the caller's side, the use of goroutines looks like this:

tasks <- func(wctx *WorkerContext) {
    // Do some worker related things with wctx.ID.

    ctx, cancel := context.WithTimeout(wctx, time.Second)
    defer cancel()

    doSomeWork(ctx)
}

Looking at the context.go code, if the given parent context is not of *cancelCtx type, it starts a separate goroutine to stick on parent.Done() channel and then propagates the cancelation.

The point is that for the performance sensitive applications starting of a separate goroutine could be an issue. And it could be avoided if WorkerContext could implement some interface to handle cancelation and track children properly. I mean something like this:

package context

type Canceler interface {
    Cancel(removeFromParent bool, err error)
}

type Tracker interface {
    AddChild(Canceler)
    RemoveChild(Canceler)
}

func propagateCancel(parent Context, child Canceler) {
	if parent.Done() == nil {
		return // parent is never canceled
	}
	if p, ok := parentCancelCtx(parent); ok { // p is now Tracker.
		if err := p.Err(); err != nil {
			// parent has already been canceled
			child.Cancel(false, err)
		} else {
			p.AddChild(child)
		}
	} else {
		go func() {
			select {
			case <-parent.Done():
				child.Cancel(false, parent.Err())
			case <-child.Done():
			}
		}()
	}
}

func parentCancelCtx(parent Context) (Tracker, bool) {
	for {
		switch c := parent.(type) {
                case Tracker:
			return c, true                
		case *timerCtx:
			return &c.cancelCtx, true
		case *valueCtx:
			parent = c.Context
		default:
			return nil, false
		}
	}
}

Also, with that changes we could even use custom contexts as children of created with context.WithCancel() and others:

type myCustonContext struct {} // Implements context.Canceler.

parent, cancel := context.WithCancel()
child := new(myCustomContext)
context.Bind(parent, child)

cancel() // Cancels child too.

Sergey.

@gopherbot gopherbot added this to the Proposal milestone Nov 11, 2018

@gopherbot gopherbot added the Proposal label Nov 11, 2018

@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Nov 28, 2018

/cc @Sajmani

I'm not convinced we want to expose the detail of the exact current Cancel signature.
In general waiting for cancellation requires a goroutine and that can be heavy (not just in this one case but in other uses too). We might want to think about that more general problem than this one short-circuit.

@Sajmani

This comment has been minimized.

Copy link
Contributor

Sajmani commented Dec 2, 2018

/cc @bcmills

Bryan has brought this up in the past, that we should make it possible for custom context implementations to avoid creating goroutines like the built-in implementation does.

In this example, it seems like the worker ID could be carried as a context value without a custom implementation. This would get you the efficiency you want at the cost of a runtime type assertion to extract the worker ID.

@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Dec 12, 2018

We know that there's no way to avoid new goroutines. Maybe that's important, maybe it's not. It's honestly unclear. Maybe things are fine as they are. The specific proposal here is probably not acceptable: we don't want to commit to exposing that specific API.

@gobwas

This comment has been minimized.

Copy link

gobwas commented Dec 23, 2018

@Sajmani seems like not only runtime type assertion, but also additional allocation to fill up interface{} header field with pointer to the, say, int ID?

@Sajmani

This comment has been minimized.

Copy link
Contributor

Sajmani commented Dec 23, 2018

@rsc rsc added the WaitingForInfo label Jan 9, 2019

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