bug: connection not cleanup up if Listener.Close() called before connection is established

[hugodutka]

The data race happens when listener.connect() is calling newDialListenerConn() and listener.Close() is checking if the connection is nil.
listener.connect() doesn't hold a mutex while establishing the connection and hasn't set the listener.cn field yet, so Close() grabs the mutex, sees that the field is nil, and sets l.isClosed to true. Once connect() establishes the connection, it's not aware that the listener has been closed and sets the listener.cn field normally, which is then never cleaned up.

[arp242]

listener.connect() doesn't hold a mutex

I'm not sure if I follow, because both Listener.connect() and Listener.Close() are protected with l.lock? In a quick test, this seems to work – I can't trigger a race anyway.

Did you experience an error? Or can you give more details?

[hugodutka]

I ran into this bug in the test suite of https://github.com/coder/coder. I'm a little hazy on the exact error I encountered then since it was a long time ago. Here's a little more detail:

because both Listener.connect() and Listener.Close() are protected with l.lock

Not all of connect() is protected with a lock:

connect()

func (l *Listener) connect() error {
    notificationChan := make(chan *Notification, 32)
    cn, err := newDialListenerConn(l.dialer, l.name, notificationChan)  // ← Network call, no lock held
    if err != nil {
        return err
    }

    l.lock.Lock()           // ← Lock acquired AFTER connection is established
    defer l.lock.Unlock()
    
    // ... resync ...
    
    l.cn = cn               // ← Connection assigned here
    l.connNotificationChan = notificationChan
    l.reconnectCond.Broadcast()
    return nil
}

Close()

func (l *Listener) Close() error {
    l.lock.Lock()
    defer l.lock.Unlock()

    if l.isClosed {
        return errListenerClosed
    }

    if l.cn != nil {        // ← Checks l.cn while connect() is dialing
        l.cn.Close()
    }
    l.isClosed = true       // ← Sets closed flag
    // ...
}

The Problem

1. listenerConnLoop() calls connect()
2. connect() calls newDialListenerConn() without holding the lock (this does network I/O)
3. While the connection is being established, Close() is called
4. Close() acquires the lock, sees l.cn == nil (connection not assigned yet), sets l.isClosed = true, and returns
5. connect() finishes dialing, acquires the lock, doesn't check l.isClosed, assigns l.cn = cn
6. The connection is now leaked — it was created after Close() decided nothing needed closing

(disclaimer: the explanation above is partly generated with Claude and later reviewed for correctness by me)

[arp242]

Not all of connect() is protected with a lock

Yeah, but it's not modifying the Listener before the lock, so that should be good?

Did you get an error from the race detector? Or did you see a logic problem (without the race detector)?

[hugodutka]

did you see a logic problem

It was a logic problem. I think the issue was that our test harness creates a database per test and then drops it after the test is finished. In tests that finished quickly and ran teardown before the connection was established, sometimes DROP DATABASE would fail because of an active connection.

If an application using pq calls connect(), and then calls Close() before the connection is fully established, the connection will be established anyway and will never be cleaned up. That's the resource leak that was causing DROP DATABASE to fail.

I mislabeled this in the original issue description. This isn't a data race (the race detector won't catch it), it's a logic race condition / resource leak. The mutex usage is correct; the bug is that connect() doesn't check l.isClosed after acquiring the lock, so a connection established during Close() is never cleaned up.