Calling Free() 2x will cause SEGV

[jezell]

Maybe should check set ptr = nil after git free and guard Free logic with ptr != nil to make sure it doesn't try to free invalid pointers?

[carlosmn]

Using any method on a wrapper object is going to cause a segfault. There isn't really anything special about the Free method.

[jezell]

Good point. Seems like the code should set ptr to nil when it is freed and handle the nil cases by returning error (and maybe ignore call in case of free), to keep with go's philosophy of not crashing because someone made a bad call somewhere.

[carlosmn]

If a developer has opted into manual resource management, I think I'd rather trust them to put the Free() at the end (which Go makes pretty convenient with defer) rather than check at every single point.

Though I guess Go doesn't panic when you try to operate on a closed file descriptor.

[jezell]

Yes. If we follow the pattern go uses for files, they check for nil in finalizer to protect and return an error if you try to double free (but not panic):

http://golang.org/src/pkg/os/file_unix.go

I believe the best practice in go is never to actually depend for finalizers to clean things up if you are a consumer of an API and always use explicit free (since there is no guarantee about when if ever the finalizer will be called), but cleaning up on finalizer is a good thing to add some degree of protection against accidental leakage.

[jezell]

Note that they do the nil check not just in the close case, so you get an ErrInvalid when you do something like invoke the function against a nil pointer to a structure as well. for instance:

var repo *Repository = nil repo.WorkDir()

Would not throw nil ref exception if this pattern was followed. Not just the case where repo.ptr is nil.

[carlosmn]

Trying to send to a closed channel will however panic, which is interesting. I guess it's because they don't have a return value.

Finalizers may not run, which is why you shouldn't release OS resources there, but in most of these structs we're dealing with memory and don't have to release any file handles. It also doesn't completely protect you, as you can still release the repository and make every other pointer invalid. This is possibly more of an argument against having a finalizer for Repository.

It's already a lot of boilerplate code everywhere to protect against use-after-free, protecting against a nil receiver doesn't feel like something anybody should be expected to do.