Bug: SemaphoreSlim being released without entering

[EricSites]

Describe the bug

at line

surrealdb.net/SurrealDb.Net/Internals/SurrealDbEngine.Ws.cs

Line 1315 in c6e1987

await _semaphoreConnect.WaitAsync(cancellationToken).ConfigureAwait(false);

internal async Task InternalConnectAsync(
        bool requireInitialized,
        CancellationToken cancellationToken
    )
    {
        if (!_wsClient.IsStarted || (requireInitialized && !_isInitialized))
        {
            try
            {
                await _semaphoreConnect.WaitAsync(cancellationToken).ConfigureAwait(false);

                if (!_wsClient.IsStarted)
                {
                    await Connect(cancellationToken).ConfigureAwait(false);
                }
            }
            finally
            {
                _semaphoreConnect.Release();
            }
        }
    }

The line await _semaphoreConnect.WaitAsync(cancellationToken).ConfigureAwait(false); should be outside of the try.

The cancellationToken token can be canceled without entering the semaphore, but the finally clause would still be called causing the semaphore to go negative.

Documentation on this here: https://learn.microsoft.com/en-us/dotnet/api/system.threading.semaphoreslim.wait?view=net-9.0#system-threading-semaphoreslim-wait(system-threading-cancellationtoken)

The code should look like:

internal async Task InternalConnectAsync(
        bool requireInitialized,
        CancellationToken cancellationToken
    )
    {
        if (!_wsClient.IsStarted || (requireInitialized && !_isInitialized))
        {
            await _semaphoreConnect.WaitAsync(cancellationToken).ConfigureAwait(false);
            try
            {
                if (!_wsClient.IsStarted)
                {
                    await Connect(cancellationToken).ConfigureAwait(false);
                }
            }
            finally
            {
                _semaphoreConnect.Release();
            }
        }
    }

Steps to reproduce

Drop the connection which will cause the cancellationToken to trigger after a long idle period.

Expected behaviour

No more logs like so:
System.Threading.SemaphoreFullException: Adding the specified count to the semaphore would cause it to exceed its maximum count.

exception_stacktrace:System.Threading.SemaphoreFullException: Adding the specified count to the semaphore would cause it to exceed its maximum count.
   at System.Threading.SemaphoreSlim.Release(Int32 releaseCount)
   at SurrealDb.Net.Internals.SurrealDbWsEngine.InternalConnectAsync(Boolean requireInitialized, CancellationToken cancellationToken) in /build/source/SurrealDb.Net/Internals/SurrealDbEngine.Ws.cs:line 1428
   at SurrealDb.Net.Internals.SurrealDbWsEngine.SendRequestAsync(String method, Object[] parameters, SurrealDbWsRequestPriority priority, CancellationToken cancellationToken) in /build/source/SurrealDb.Net/Internals/SurrealDbEngine.Ws.cs:line 1449
   at SurrealDb.Net.Internals.SurrealDbWsEngine.Select[T](RecordId recordId, CancellationToken cancellationToken) in /build/source/SurrealDb.Net/Internals/SurrealDbEngine.Ws.cs:line 871```

### SurrealDB version

SurrealDB 2.2.1

### Package version(s)

latest

### Contact Details

esites@calamu.com

### Is there an existing issue for this?

- [x] I have searched the existing issues

### Code of Conduct

- [x] I agree to follow this project's Code of Conduct

[Odonno]

Hello Eric,

I got into the code and it indeed abort early by returning a Task.FromCanceled if the cancellation token is already canceled. Good catch.

Do you want to make a PR for this?

[Odonno]

But still, the problem comes frome the Release call. So, shouldn't we prevent continuation of the method, kind of like this:

var task = _semaphoreConnect.WaitAsync(cancellationToken);
await task.ConfigureAwait(false);

if (task.IsCanceled)
{
  return;
}

[EricSites]

No, because an exception is thrown.

"If cancellationToken is cancelled, the thread or task doesn't enter the semaphore, and the CurrentCount property isn't decremented. Instead, the method throws an OperationCanceledException exception."

[Odonno]

Ah, right. It will indeed throw an Exception and as it in a try/catch, it will release even though the Semaphore wasn't properly entered. Just like you said.

Do you wish to make a PR for this?