proposal: runtime: introduce convention of using SetFinalizer to help detect resource leaks

[akavel]

Rationale

Inspired by a recent comment on r/golang by BowsersaurusRex:

In C# I'll often use finalizers to track bugs in which an object was GC'd but Close() was never called.

and the post by @crawshaw which led to it, I started to think that maybe the idea quoted above (to use finalizers to help detect resource leaks) could be promoted into a "good practice", with some support in the standard library. It's just an idea, and quick googling didn't lead me to any previous discussion of a similar proposal (though see Related Work below), so I thought to open an issue to start a discussion and explore whether it could be seen as reasonable and advisable.

Proposal

As a quick first draft, I propose the following new variable is added in package runtime:

package runtime

// Leaks is a channel where resource leaks detected by packages are sent
// on best-effort basis. Errors received from this channel can help pinpoint
// resource leaks such as missing File.Close calls.
//
// TODO: example code showing SetFinalizer usage pattern
var Leaks = make(chan error)

and then for example os.File's finalizer is changed from current .close call:

runtime.SetFinalizer(f.file, (*file).close)

to something like below:

runtime.SetFinalizer(f.file, (*file).leak)

where leak could be implemented as:

func (file *file) leak() {
    select {
    case runtime.Leaks <- errors.New(file.name + ": missing Close):
    default:
    }
    file.close()
}

(Discussion thread on golang-nuts)

Related Work

• golang-nuts: Resource leak detection by @dvyukov

[Merovius]

As mentioned on golang-nuts, I don't really think this needs to be provided as part of stdlib. Here is a simple implementation as a regular package, that can be used to augment any io.Closer (and by extension, anything) with this functionality:

package leak

import (
	"io"
	"runtime"
	"sync"
)

func TrackLeaks(c io.Closer, f func(io.Closer)) io.Closer {
	x := &closer{c: c}
	runtime.SetFinalizer(x, func(c *closer) {
		if c.c != nil {
			f(c.c)
		}
	})
	return x
}

type closer struct {
	o   sync.Once
	c   io.Closer
	err error
}

func (c *closer) Close() error {
	c.o.Do(func() {
		c.err = c.c.Close()
		c.c = nil
	})
	return c.err
}

[akavel]

@Merovius

1. The idea would be that package authors would use it when allocating new resources. How do you propose an author of a package, let's say "somedatabase", would use TrackLeaks in somedatabase.Connect(string) (somedatabase.Conn, error)?
2. My main idea for including this in stdlib was to: (a) make stdlib also use this mechanism (os.File, net/http.Response.Body, etc.); (b) obviously as usual, to get "free publicity", alias one standard mechanism. While (b) can be argued with, and usually the way to (a) and (b) is through a third-party package, I was curious what the core devs would think about the idea, and whether they might like it enough to jump over the long and erratic way through a third-party package, and straight to (a) & (b). Regardless, I sure wanted to open discussion and solicit people's thoughts about this idea anyway. Or to find out if it was considered already and rejected. I might try the route through a third-party pkg if advised so by the core team.

EDIT:

Ok, actually the third party package could be basically:

package leak
var Found = make(chan error)

with proper godoc, examples, etc. If advised so by the core team, and if that's not the contents of David Crawshaw's alluded "followup blog post" (assuming it gets published soon enough edit: published already), I plan to try and create such a package (with proper docs) and advertise it on golang-nuts + reddit.

EDIT 2: David Crawshaw's followup blog post seems to suggest panicking. I believe a channel like described above is a nice generic mechanism, which can be then handled however the end-user wants, in a fully and purely opt-in way:

• a) ignored (default behavior)
• b) dumped to stderr
• c) logged through app's chosen usual mechanism
• d) panicked (as in @crawshaw's post)

EDIT 3: Hm; how about golang.org/x/leak or ...x/tools/leak or something like this? Would such a location be accepted? If prototyping goes well, in later version this could probably be migrated to stdlib in similar vein as pkg context.

[Merovius]

The idea would be that package authors would use it when allocating new resources.

Why? I mean, why does it have to be this way? If you don't rely on package authors, but provide the generic functionality (and really, runtime.SetFinalizer already does that, but that's not the point), then a) package authors can do it, but b) if they don't you as a person who cares about it can still do it anyway.

How do you propose an author of a package, let's say "somedatabase", would use TrackLeaks in somedatabase.Connect(string) (somedatabase.Conn, error)?

The API as it stands is explicitly geared towards the user of the package. The package author could use the API as it stands by taking an appropriate function from the user, use a package-level variable (akin to your Leak) to let the user specify such a function, just use a fixed one… Or you can change the API to fit a different pattern that is better geared towards usage by package authors (like the one you proposed above, though I'd still argue in favor of a func, instead of a chan, as it's easier to use, less overhead and more powerful).

My basic point was that - questions of good API design aside - the functionality doesn't have to live in the stdlib (or the runtime package) to be useful. If anything, the breadth of the design space for the API is an argument to not put it into the stdlib, but instead experiment with 3rd party packages first, to converge on good patterns.

[akavel]

This would be available to package users too, sure. But as to why the package authors are the main target, the reason is similar to why SetFinalizer is used in stdlib. If you're the user, you generally have defer, so you don't need the SetFinalizer. If you forgot about defer, why wouldn't you forget about SetFinalizer. But if you're author, you cannot force user to call your Close, the user must remember about the defer. But if there was one common funnel where all packages could report leaks, the end-user could plug into it and get reports, and SetFinalizer would provide value (of "early-ish detection of leaks").

Using func in the API is problematic because of concurrency. I started with idea of a func, but then the user can't easily replace the var. With chanel, one can just listen or not. As to "overhead", should be negligible compared to "heaviness" of resource usage. Also, it's the pessimistic path when you have a leak anyway, in optimistic path you clear it with SetFinalizer(..., nil). So I don't see performance as a problem here.

As to stdlib or not, I don't see point in us arguing over this any more, I'm interested in opinions of core devs, it's up to them anyway. In this regard I'm also interested if golang.org/x could be a fit.

[RLH]

Like finalizers this would asynchronously fired bits of code that may or may not ever run. If there are bugs in your finalizer code they are almost always going to be non-deterministic, potentially racy in ways the race detectors don't detect, and almost always hard to impossible to debug. Furthermore writing a deterministic test based on a non-deterministic construct is a challenge. Sometimes the test reports a leak, sometimes it doesn't, sometimes there is a race, sometimes there isn't, it worked in Go 1.N but it doesn't in 1.N+1, and on and on. Hide the construct in a packages that would otherwise be opaque and you have a foot gun. This is not the way forward.

…

On Thu, Apr 5, 2018 at 9:36 AM, Axel Wagner ***@***.***> wrote: The idea would be that package authors would use it when allocating new resources. Why? I mean, why does it *have* to be this way? If you don't rely on package authors, but provide the generic functionality (and really, runtime.SetFinalizer already does that, but that's not the point), then a) package authors *can* do it, but b) if they don't you as a person who cares about it can still do it anyway. How do you propose an author of a package, let's say "somedatabase", would use TrackLeaks in somedatabase.Connect(string) (somedatabase.Conn, error)? The API as it stands is explicitly geared towards the user of the package. The package author *could* use the API as it stands by taking an appropriate function from the user, use a package-level variable (akin to your Leak) to let the user specify such a function, just use a fixed one… Or you can change the API to fit a different pattern that is better geared towards usage by package authors (like the one you proposed above, though I'd still argue in favor of a func, instead of a chan, as it's easier to use, less overhead and more powerful). My basic point was that - questions of good API design aside - the *functionality* doesn't have to live in the stdlib (or the runtime package) to be useful. If anything, the breadth of the design space for the API is an argument to *not* put it into the stdlib, but instead experiment with 3rd party packages first, to converge on good patterns. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#24696 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA7Wn2cbmuGexLiTk79rcP-Mhk2sjHGoks5tlh3QgaJpZM4TIRLi> .

[akavel]

@RLH I imagine the API would be basically just sending a string to the channel. I kept the 'close' in the original example only because it was already there; in common case, I'd advise to leave it out. If I put it in a third-party pkg (or golang.org/x), I believe I'd add a helper function, so that a typical finalizer could look like:

func NewFoo() *Foo {
    // ... build foo ...
    runtime.SetFinalizer(foo, (*Foo).leak)
    return foo
}
(f *Foo) leak() { leak.Report(f.String() + ": missing Close") }
(f *Foo) Close() error {
    // ... do the main Close business ...
    runtime.SetFinalizer(f, nil)
    return nil
}

I don't see how this introduces any new problems with races, debugging etc., not present yet in the pre-existing code?

EDIT: Ok, I see @RLH's argument as a valid point against putting this in stdlib, to that extent I see the potential for ensuing mess. How about a proposal for an experiment with a narrow API in golang.org/x then? Could it have a chance of being accepted? Let's say:

package leak
var Found <-chan error
func Report(string)

// optionally two extra helpers:
func Mark(obj interface{}, message string)    // calls SetFinalizer with a func calling: Report(message)
func Unmark(obj interface{}) 

[Merovius]

Using func in the API is problematic because of concurrency. I started with idea of a func, but then the user can't easily replace the var. With chanel, one can just listen or not.

Are you suggesting that multiple consumers should read from the same channel (doing presumably different things)? Because that seems like a bad idea.

[rsc]

Anyone interested in this topic should read Hans Boehm's paper Destructors, Finalizers, and Synchronization. It greatly influenced our decision to limit the scope of finalizers in Go as much as possible. They are a necessary evil for allowing reclamation of non-(heap memory) resources at the same time as associated heap memory, but they are inherently much more limited in their capabilities than most people initially believe. We will not be expanding the scope of finalizers, either in implementation or in the standard library, or in the x repos, nor will we encourage others to expand that scope.

If you want to track manually-managed objects, you'd be far better off using runtime/pprof.NewProfile. For example, inside Google's source tree we have a Google-wide "file" abstraction, and the Go wrapper package for this declares a global:

var profiles = pprof.NewProfile("file")

The end of the function that creates a new File says:

profiles.Add(f, 2)
return f

and then f.Close does

profiles.Remove(f)

Then we can get a profile of all in-use files, "leaked" or otherwise, from /debug/pprof/file or from pprof.Lookup("file").WriteTo(w, 0). And that profile includes stack traces.