Proposal Details
If the function passed to runtime.AddCleanup contains a reference to the variable that the cleanup is attached to, then the variable will remain live and the cleanup will never run. A recent CL exhibited this bug and I can see it being a fairly easy mistake to make. Since AddCleanup is a relatively new API addition, I think it makes sense to detect these errors now before they become a problem.
I suggest that vet warn about this problem. I have an implementation for gopls at CL696775 that can be used to evaluate just how helpful this warning would be.
Comment From: DanielMorsing
cc @alandonovan @golang/tools-team
Comment From: gabyhelp
Related Issues
(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)
Comment From: prattmic
cc @mknyszek @golang/runtime
Comment From: mknyszek
For the tools folks: I think this is the kind of thing that if we can catch any cases it's useful, and there won't be any false positives. This mistake will always cause the cleanup to never execute, which is essentially never what the user of AddCleanup wants.
Comment From: DanielMorsing
For some reason, I remembered runtime.SetFinalizer doing something special with root marking, but some testing I did shows that it's susceptible to the same issue. Should I expand this proposal to also include SetFinalizer?
Comment From: mknyszek
Possibly. SetFinalizer has indeed had this problem since forever. There is one risk of false positive that I'd be more worried about enabling with SetFinalizer.
You could imagine if the closed over variable only transitively refers to the object that the program intentionally deletes that pointer in order to make the object collectible. This isn't recommended and anyone trying to rely on this is essentially asking for a memory leak bug, but it may be the case that real code does this. AddCleanup is new, so there's a much lower chance that code like this exists. SetFinalizer is old and the vet check may start giving errors about what some users see as "benign" code. Again, I don't think people should do this, but the bar for a vet check is very high.
Comment From: ianlancetaylor
What if we instead do something like https://go.dev/cl/697535? It seems like that wouldn't have any false positives, and I suspect it would catch almost as many cases as a vet check. That is, clearly there are many cases that that CL won't catch, but I think it would be hard for vet to catch those cases also.
Comment From: gopherbot
Change https://go.dev/cl/697535 mentions this issue: runtime: panic on AddCleanup with self pointer