gogo-context

Why is it always bad practice to skip calling the cancel function for a child context?


Problem Description

When using the "context" package in Go, the go vet command's "lostcancel" check will give a warning if you don't call the cancel function returned by context.WithCancel(). e.g. if you have:

func main() {
    ctx, cancel := context.WithCancel(context.Background())
    foo(ctx)
    fmt.Println("done")
}

...then Go vet would give you the warning:

the cancel function is not used on all paths (possible context leak)

This is also mentioned in the Context package's documentation:

Failing to call the CancelFunc leaks the child and its children until the parent is canceled or the timer fires. The go vet tool checks that CancelFuncs are used on all control-flow paths.

My issue is this: in at least one instance, I don't see why the cancel function should have to be called. Suppose I want to derive a child context so that I have the option to either:

  1. stop downstream goroutines using the child context before the parent is canceled, such as upon encountering an early error, or
  2. wait until the parent context is canceled, letting the child context live as long as the parent.

This seems to me to be a very natural use-case of the context package's parent/child tree structure. But this gives the same go vet error as mentioned above, because in the code execution path representing the second option (letting the child live for the same duration as the parent), the child context's cancel function wouldn't be called.

So, is this truly a shortcoming of go vet/the Go community's guidelines for using Context, or is there a flaw in my thinking about this topic?

Example Program

Here is a sample program where the error occurs. Go vet would give the warning for the line barctx, barcancel := context.WithCancel(ctx). This example is obviously silly, because I could just check bar for an error before starting up its goroutine. But suffice it to say: in my actual program, this would not be an option.

func main() {
    ctx, cancel := context.WithCancel(context.Background())
    foo(ctx)
    cancel()

    time.Sleep(10 * time.Millisecond)
    fmt.Println("done")
}

func foo(ctx context.Context) {
    go func() {
        <-ctx.Done()
        fmt.Println("foo done")
    }()

    for i := 0; i < 2; i++ {
        barctx, barcancel := context.WithCancel(ctx)
        err := bar(barctx, i)
        if err != nil {
            fmt.Println("bar error; canceled")
            barcancel()
        }
    }
}

func bar(ctx context.Context, x int) (err error) {
    go func() {
        <-ctx.Done()
        fmt.Printf("bar done; x=%d\n", x)
    }()

    if x > 0 {
        err = fmt.Errorf("bar error")
    }
    return err
}

Solution

  • In your example, go vet would be correct to return a warning. The problem specifically lies in the bar function. When you create it, bar creates a goroutine that blocks until barcancel or cancel is called. Until then, it will continue to exist. Therefore, it is imperative that you call the cancel function so that resources waiting for the context to be done can go out of scope and be garbage-collected.

    To be clear, in your example, all the deferred routines will be cancelled because you cancelled ctx, which also cancelled every instance of barctx. However, you shouldn't depend on this as you might not have control over whether or not the parent context will be cancelled. So, your function should take responsibility for the contexts it creates and uses in function invocations.

    To be clear, you should modify your example code like this:

    barctx, barcancel := context.WithCancel(ctx)
    err := bar(barctx, i)
    if err != nil {
        fmt.Println("bar error; canceled")
    }
    
    barcancel()