Skip to content

Fix leaky semaphore in Connection.WriteInternalAsync #936

Description

@jpdillingham

The write path of a Connection maintains a semaphore to track the number of messages waiting to be written to the socket. If this semaphore ever drains completely, the connection disconnects with the message "The write buffer is full". This is designed to prevent slow distributed children from bogging the application down; otherwise we keep adding to the stack of waiting Tasks until we run out of memory.

There's a problem with the implementation of this check:

            // in the case of a bad (or failing) connection, it is possible for us to continue to write data, particularly
            // distributed search requests, to the connection for quite a while before the underlying socket figures out that it
            // is in a bad state. when this happens memory usage skyrockets. see https://github.com/slskd/slskd/issues/251 for
            // more information.  note that this isn't for synchronization, it's to maintain a count of waiting writes.
            if (WriteQueueFull || !await WriteQueueSemaphore.WaitAsync(0, cancellationToken).ConfigureAwait(false))
            {
                // note: the semaphore check and this latch are not atomic! it's possible for one thread to fail to get the semaphore,
                // and for the next to succeed (if one is released in the finally) before we set this to true. if that happens,
                // *something* below will fail and the exception will bubble out (and if it doesn't throw, it probably got sent)
                WriteQueueFull = true;

                Disconnect("The write buffer is full");
                throw new ConnectionWriteDroppedException($"Dropped buffered message to {IPEndPoint}; the write buffer is full");
            }

            // obtain the write semaphore for this connection.  this keeps concurrent writes
            // from interleaving, which will mangle the messages on the receiving end
            await WriteSemaphore.WaitAsync(cancellationToken).ConfigureAwait(false);

Both of these semaphores are released in the finally block, so it's not leaking for every write. If WriteSemaphore.WaitAsync(cancellationToken) throws (not sure how!) or is cancelled, WriteQueueSemaphore is never released.

In practice, this is a non-issue because if a write is cancelled, the connection is almost certainly being or is about to be torn down, making the leaked semaphore a non issue. This is a bug though and it should be fixed.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions