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
Comment From: mknyszek
@ianlancetaylor Checking at runtime is a good idea too. Though correct me if I'm wrong but I think that particular CL wouldn't help in this particular reported issue, because the value wasn't passed as an argument to the cleanup, but was instead closed over by the cleanup function.
We could check if the cleanup function has a closure allocation, look it up in the heap (it must already escape) and walk its pointers to see if they point into the provided object. Unfortunately, that might end up being somewhat expensive.
Comment From: ianlancetaylor
Ah, you're right, it wouldn't help with this case. Ah well.
Comment From: DanielMorsing
Disallowing functions to have closures at all might have been a decent choice, but now that it's shipped, I don't think we can reasonably do that.
Comment From: ianlancetaylor
It turns out to be pretty simple to catch the case where the cleanup function refers to the pointer with the cleanup. I did that in https://go.dev/cl/698595 on top of the earlier https://go.dev/cl/697535. This does catch the case mentioned above in https://go.dev/cl/696295.
Comment From: gopherbot
Change https://go.dev/cl/698595 mentions this issue: runtime: panic if cleanup function closes over cleanup pointer
Comment From: DanielMorsing
If this can be checked at runtime, I don't think the vet implementation has much merit. We should close this issue once the runtime check lands.
Comment From: mknyszek
@ianlancetaylor That's essentially what I had in mind, but I am slightly worried about the performance overhead. I mean, it's probably fine, at least in all the places AddCleanup is used today.
The performance impact of the check might also discourage folks from closing over anything in the first place and actually using the argument as intended (which we can, in the future, store without an explicit allocation in some cases, like if it's just a single word in size).
Comment From: mknyszek
We may need a proposal to start panicking in this case, and maybe a compatibility GODEBUG just to be super safe on backwards compatibility, but it hasn't been too many releases since AddCleanup, so I'm in favor of just adding the extra checks if others are as well. But that's another reason I thought a vet check might be better (and it would be the only option if we don't feel good about these new panics.)
I'll also note the runtime check approach suffers from the same potential problem as above:
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.
RE: my concerns about performance, they're largely motivated by the unique package, which does actually create a closure, since it needs access to two different things: the canonicalization map itself, and the weak pointer to look up the value and destroy it. But this is on the insert path in unique, which is not the hottest path. So again, it's probably fine, and we should perhaps just stop creating a closure there and optimize some other aspects of cleanups, like I mentioned earlier.
Comment From: ianlancetaylor
Personally I don't think we need a proposal or a GODEBUG for adding a panic. There shouldn't be any false positive here—this is just fixing broken code.
I would be surprised if the performance hit is noticeable.
I don't feel strongly about any of this. It's really up to you.