samuong / alpaca

A local HTTP proxy for command-line tools. Supports PAC scripts and NTLM authentication.
Apache License 2.0
184 stars 31 forks source link

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

Closed samuong closed 4 years ago

samuong commented 4 years ago

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 commented 4 years ago

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- commented 4 years ago

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 commented 4 years ago

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- commented 4 years ago

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.

samuong commented 4 years ago

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