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

cmd/vet: govet sometimes wrong about whether a context gets cancelled #29587

Open
the80srobot opened this issue Jan 6, 2019 · 10 comments

Comments

@the80srobot
Copy link

@the80srobot the80srobot commented Jan 6, 2019

What version of Go are you using (go version)?

Not sure, it's a toolchain. Most likely 1.11.

Does this issue reproduce with the latest release?

Yes, as of 1.12beta.

What operating system and processor architecture are you using (go env)?

No idea, govet runs remotely on a language server.

What did you do?

ctx, cancel := context.WithCancel(context.Background())
defer cancel()
if true {
  // This makes govet think that the child context never gets cancelled,
  // when, in fact, it is cancelled by the parent's cancel().
  ctx, _ = context.WithTimeout(ctx, time.Second)
}

What did you expect to see?

govet should not claim that the child context leaks. (If it's a style question, it should be in the linter, not govet.)

What did you see instead?

govet incorrectly thinks the child context does not get cancelled.

@ianlancetaylor ianlancetaylor changed the title govet sometimes wrong about whether a context gets cancelled cmd/vet: govet sometimes wrong about whether a context gets cancelled Jan 6, 2019
@ianlancetaylor ianlancetaylor added this to the Go1.13 milestone Jan 6, 2019
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jan 6, 2019

(I edited the submission to fix the formatting of the Go code.)

@josharian

This comment has been minimized.

Copy link
Contributor

@josharian josharian commented Jan 7, 2019

@mvdan

This comment has been minimized.

Copy link
Member

@mvdan mvdan commented Jan 7, 2019

Thankfully not a regression from 1.11 :) A conservative fix here might be simple enough to add to 1.12.

@mvdan

This comment has been minimized.

Copy link
Member

@mvdan mvdan commented Jan 7, 2019

Seems to me like the simplest conservative fix would be to turn off this warning in the general ctx2, _ := context.WithFoo(ctx, ...), where we know ctx not to be the background context. However, this analyzer is a bit complex, so I'll let @alandonovan decide what the best fix would be.

@alandonovan

This comment has been minimized.

Copy link
Contributor

@alandonovan alandonovan commented Jan 7, 2019

the simplest conservative fix would be to turn off this warning in the general ctx2, _ := context.WithFoo(ctx, ...), where we know ctx not to be the background context.

That would substantially diminish the value of the check: it's probably the common case that ctx is not Background.

This is one of those cases in which a higher-level invariant of the program that is beyond the scope of the tool ensures that the apparent problem is benign. I suggest adding a workaround:

ctx, cancel := context.WithCancel(context.Background())
defer cancel()
if true {
  ctx, unused = context.WithTimeout(ctx, time.Second)
  _ = unused // pacify vet lostcancel check: ctx is always canceled through its parent
}
@mvdan

This comment has been minimized.

Copy link
Member

@mvdan mvdan commented Jan 7, 2019

I'm not sure I agree. The program initially posted is correct in my mind, so I don't see why it should be annotated. Seems to me like it's vet that should get smarter about false positives.

Unless you're talking about the more general case, where we don't know if the parent cancel func is called properly:

func Foo(ctx context.Context) {
    // do all the Foo callers cancel ctx above properly?
    ctx2, _ := context.WithTimeout(ctx, time.Second)
    Bar(ctx2)
}

I'd probably agree that in the generic case where you can't know if the parent will be cancelled properly, the child's cancel func being discarded should be very obvious. But the program originally posted has no such ambiguity.

@alandonovan

This comment has been minimized.

Copy link
Contributor

@alandonovan alandonovan commented Jan 7, 2019

Yes, the program is correct and the diagnostic is a false positive, but vet checks are nearly all heuristics, so we can't realistically fix every false positive. There has to be a cost/benefit calculation. In this example, the relationship between the two contexts is evident within the body of a single function, so it's probably feasible to expand the scope of the algorithm. However: it's already not a trivial piece of logic; minor refactorings could easily obscure this relationship; and this is the first report of this problem we've had in 2.5 years since the check was added, and there's an easy workaround. So I'm inclined not to spend more effort on it yet.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jan 7, 2019

Marking as "help wanted", to leave the issue to someone who cares enough to write a fix.

@mvdan

This comment has been minimized.

Copy link
Member

@mvdan mvdan commented Jan 8, 2019

this is the first report of this problem we've had in 2.5 years since the check was added, and there's an easy workaround. So I'm inclined not to spend more effort on it yet.

That's a good point. I hadn't realised that the lostcancel check was like this for years.

@xanderflood

This comment has been minimized.

Copy link

@xanderflood xanderflood commented Apr 8, 2019

I have a similar issue that shows up when using a BDD test tool like ginkgo. The standard practice there is do setup and teardown in separate contexts, meaning you might have:

BeforeEach(func() {
    ctx, cancel = context.WithCancel(context.Background)
}

JustBeforeEach(func() {
    output, err = FunctionUnderTest(ctx)
})

AfterEach(func() {
    cancel()
})

If the function under test is synchronous, then you can put both of these steps into JustBeforeEach and satisfy go/vet, but if FunctionUnderTest is intended to still be running during your assertions, that's not an option. Any thoughts on workarounds for this situation? Alternatively, is there any way run go/vet but to disable this specific check? It'd be nice to be able to do that when linting test files (but of course this is a useful checl that I'd want on my actual code).

@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
XORveRCOM added a commit to XORveRCOM/util that referenced this issue Nov 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.