Is cancel so mandatory for context?

Question

Got this from go vet:

the cancel function returned by context.WithTimeout should be called, not discarded, to avoid a context leak

The code is as follows:

func foo(ctx context.Context) {
  for ... some loop {
    c, _ := context.WithTimeout(ctx, time.Second)
    err = enc.RequestWithContext(c, "some", someRequest, &response)
    if err != nil {
      continue
    }
    if response.Code == -1 {
      break
    }
  }
}

So what is the point of cancelling it here after request? Just to make vet happy? That context is anyway already either finished or timed out.

func foo(ctx context.Context) {
  for ... some loop {
    c, cancel := context.WithTimeout(ctx, time.Second)
    err = enc.RequestWithContext(c, "some", someRequest, &response)
    cancel()

    if err != nil {
      continue
    }
    if response.Code == -1 {
      break
    }
  }
}

"That context is anyway already either finished or timed out." That's the point - you can't be certain of that unless you cancel it when you're done with it. — sdgluck, Jan 22 '20 at 11:03
How I can't be sure if function returns already? It's synchronous. — Alex, Jan 22 '20 at 11:06
If your request finishes in less than a second the context is not going to be canceled yet, and binds resources for the remaining time of that second. With enough concurrent requests that matters and that's why vet points it out. — Peter, Jan 22 '20 at 11:15

icza · Accepted Answer · 2020-01-22T11:17:45.290

c, cancel := context.WithTimeout(ctx, time.Second)
err = enc.RequestWithContext(c, "some", someRequest, &response)
cancel()

enc.RequestWithContext() may return normally, before the 1-second timeout, and the resources used by the context will only be freed immediately if you call cancel().

cancel() is idempotent, you may call it multiple times, concurrently from multiple goroutines. Subsequent calls are a noop. If the context has already timed out or cancel() has been called, calling it (again), does no harm.

Making sure cancel is called is easiest done with defer, something like this:

c, cancel := context.WithTimeout(ctx, time.Second)
defer cancel()
err = enc.RequestWithContext(c, "some", someRequest, &response)

But be careful when this snippet is inside a loop: deferred functions are only executed when the surrounding function ends, not when the next loop iteration begins. In such cases you should use a function literal or a named function to make sure deferred calls are executed before the next iteration begins. For details, see `defer` in the loop - what will be better?

A possible solution here would be:

for ... some loop {
    res := func() bool {
        c, cancel := context.WithTimeout(ctx, time.Second)
        defer cancel()
        err = enc.RequestWithContext(c, "some", someRequest, &response)
        if err != nil {
            return false
        }
        if response.Code == -1 {
            return true
        }
    }()
    if res {
        break
    }
}

Defer in a loop is kinda weird. Also, there is no reason to keep that context because operation already finished, the request returned either error or response. The purpose of that context with timeout here is to wrap parent context (so request will be cancelled in case parent exits and set a timeout on `.RequestWithContext` call. — Alex, Jan 22 '20 at 10:59
@Alex Please check the linked answer. It details how `defer` should be used inside a loop. — icza, Jan 22 '20 at 11:01
@Alex I misinterpreted your question, I'm editing the answer. — icza, Jan 22 '20 at 11:12
Marking as answer. You confirmed what I have been thinking about. — Alex, Jan 22 '20 at 12:40

score 5 · Answer 2 · answered Jan 22 '20 at 11:18

Is cancel so mandatory for context?

Yes, no arguing here.

From go doc context:

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.

Failing to cancel is so bad that even go vet warns you about it.

Is cancel so mandatory for context?

2 Answers2