Introduce a cancelable.Closer type to manage net.Conn lifetimes

[samuong]

The ProxyHandler.handleConnect() and connectViaProxy() functions have a lot of early returns due to error handling, and each of these error handling blocks need to call net.Conn.Close() on various connections. Unfortunately, they can't use the standard defer idiom, because in the success case these functions will pass their net.Conns to other functions. As a result, there are a lot of manual calls to net.Conn.Close(), and it's easy to forget to do this (and I'm pretty sure I did in a few places).

This pull request introduces a new type called cancelable.Closer, which is modelled on C++11's std::unique_ptr. It holds "ownership" of an io.Closer, and allows users to use the defer idiom to call the Close() function. But once we've passed all the error handlers, a user can call Cancel(), which will release ownership of the io.Closer, so that it can be returned or passed to another function/goroutine. It is safe to call cancelable.Closer.Close() multiple times.

A typical usage might look like this:

conn := newConn()
closer := cancelable.Closer(conn)
defer closer.Close()
if err := doSomeStuff(); err != nil {
    // early exit; conn will be closed by the cancelable.Closer
    return nil, err
}
// release ownership so that we can return the conn
closer.Cancel()
return conn, nil
// the defered call to closer.Close() will still happen, but doesn't do anything

[samuong]

I'm interested to know what you all think of this. It's an attempt to bring C++ smart pointer style resource management to Go. I think it does make things easier to manage, but I'm not sure if this really counts as "idiomatic Go"; maybe there are more idiomatic ways to write these functions so that I would have avoided the need for a ResetCloser in the first place? Please take a look and let me know what you think.

Although I generally prefer to squash merge and preserve simple linear histories, for this PR I've followed @camh-'s style of having multiple atomic commits in a single PR. This separates the changes to error logging from the ResetCloser changes, so please review this one commit at a time.

[camh-]

I really like the idea - no comment on whether it is too C++ like - if it works, and makes for simpler more robust code, then go for it.

With all my comments, I ended up writing my own to see what it would look like with those comments addressed. Here's what I came up with:

// ErrCloser wraps an io.Closer with its own Close() method that ensures
// that the wrapped io.Closer is closed at most once and not at all if
// Release() is called before Close(). It is intended to be used with
// defer so the io.Closer is closed on the error paths out of a function,
// but the success path can call Release() to ensure that Close() does
// not actually close the io.Closer.
type ErrCloser struct {
	io.Closer
}

// Release releases the wrapped io.Closer so that we will no longer try to
// close it when Close() is called.
func (ec *ErrCloser) Release() {
	ec.Closer = nil
}

// Close closes the wrapped io.Closer if it has not already been closed by
// this ErrCloser. Close will release the wrapped io.Closer so this ErrCloser
// will not try to close it again.
func (ec *ErrCloser) Close() error {
	if ec.Closer == nil {
		return nil
	}
	if err := ec.Closer.Close(); err != nil {
		return err
	}
	ec.Release()
	return nil
}

I didn't test it - I just wrote it in the playground and it compiles, so it is ready to be shipped.

[edit: updated to make receivers to be pointers]

[camh-]

Here's an alternate idea that is a little more light weight (see also at https://goplay.space/#l_2QKOXTkGK):

type CancelableFunc func()

func (cf *CancelableFunc) Cancel() {
	*cf = nil
}

func (cf *CancelableFunc) Call() {
	if *cf != nil {
		(*cf)()
		*cf = nil
	}
}

Use it like:

closer := CancelableFunc(func() {conn.Close()})
defer closer.Call()
...
closer.Cancel()

[edited to simplify Call() a bit more]

[samuong]

Here's an alternate idea that is a little more light weight (see also at https://goplay.space/#l_2QKOXTkGK):

I was thinking of doing something similar earlier - the generic (not limited to io.Closers) version of this would allow people to cancel any defered closure. It does make it more lightweight to implement, but the only thing I didn't like was that it was more code to construct in what I think is the common case, i.e. closer := CloseCanceler{conn} vs closer := CancelableFunc(func() { conn.Close() }.

I'm thinking that cancelling a Close() is by far the most common use case for this, and maybe Lock/Unlock would be the other ones. What if this was split out into a generic package called cancelable, with the following types?

1. Closer
2. Locker
3. Unlocker
4. Func

[camh-]

I like the naming by using the package cancelable and then just using Closer inside that.

I'm not sure for the use case of Locker and Unlocker. locks don't usually outlast the scope of a function like a connection would. And Func is not really needed in this case if you have the Closer. But conceivably the package could be extended with those if they became needed.

But I like this idea best and would be happy with just cancelable.Closer to start with.

[camh-]

this PR has changes from your other PR (#28). Can you rebase it onto that branch and make that branch the base for this PR? It's hard to review the changes that are relevant to just this PR.

[samuong]

Now that I've merged #28, I've just rebased this on top of master (and addressed your comment about adding an error field).

[camh-]

LGTM