Deadlock when opening and closing connections concurrently

[joao-r-reis]

I ran into two different issues, one is a deadlock and the other is leaked goroutines. I'm not exactly sure if these goroutines will be cleaned at some point but I did see them in the debugger.

Deadlock

While trying to reproduce #36 with an automated test, I ran into a deadlock caused by a blocking channel write here that causes the goroutine to not release the connectionsLock.

In the debugger I see a lot of goroutines blocked while trying to acquire that lock which is not possible because of that blocked channel write in the first goroutine.

I believe that this is because that channel is in fact nil due to this call to close() and writing to a nil channel blocks forever.

Leaked goroutines

I also see several background goroutines blocked here and here.

This happens because neither channel is buffered so the write will only progress when a read happens but the for loop returns before reading from both when an error happens.

---

Here is the test that I used to reproduce these issues:

func TestDeadlock(t *testing.T) {
    server := client.NewCqlServer(
        "127.0.0.1:9043",
        &client.AuthCredentials{
            Username: "cassandra",
            Password: "cassandra",
        },
    )
    defer server.Close()

    clt := client.NewCqlClient(
        "127.0.0.1:9043",
        &client.AuthCredentials{
            Username: "cassandra",
            Password: "cassandra",
        },
    )

    numberOfGoroutines := 10
    errChan := make(chan error, numberOfGoroutines)
    wg := &sync.WaitGroup{}
    defer wg.Wait()
    ctx, cancelFn := context.WithTimeout(context.Background(), 5 * time.Second)
    defer cancelFn()

    err := server.Start(ctx)
    require.Nil(t, err)

    for i := 0; i < numberOfGoroutines; i++ {
        wg.Add(1)
        go func() {
            defer wg.Done()
            for ctx.Err() == nil {
                newCtx, newCancelFn := context.WithTimeout(ctx, time.Duration(rand.Intn(50)) * time.Millisecond)
                clientConn, serverConn, err := server.BindAndInit(
                    clt, newCtx, primitive.ProtocolVersion4, client.ManagedStreamId)
                if err == nil {
                    err = clientConn.InitiateHandshake(primitive.ProtocolVersion4, client.ManagedStreamId)
                }
                newCancelFn()
                if clientConn != nil {
                    clientConn.Close()
                }
                if serverConn != nil {
                    serverConn.Close()
                }
                if err != nil && newCtx.Err() == nil {
                    errChan <- err
                    return
                }
            }
        }()
    }

    select {
    case err = <-errChan:
        t.Errorf("%v", err)
    case <-ctx.Done():
    }
}

[adutra]

1. For the deadlock, I think we simply need to test h.isClosed() after acquiring h.connectionsLock in onConnectionAccepted.
2. Leaked go routines: the code used to test if an error occurred is indeed clumsy. I suggest that we replace it with https://pkg.go.dev/golang.org/x/sync/errgroup. We used it already in another project and it works great, even if it does not have a stable version yet.

[joao-r-reis]

For 1, won't that introduce a race condition?

[adutra]

I think we are fine? Because the channel is closed while holding the connectionsLock, see here.