Race condition with MySql

[trejjam]

Confirm you've already contributed to this project or that you sponsor it

• I confirm I'm a sponsor or a contributor

Version

4.x

Describe the bug

Hi,
I may be hitting a race condition while revoking tokens in OpenIddictServerHandlers.Protection.cs:ValidateTokenEntry.HandleAsync.TryRevokeChainAsync.
We are using a MySql server, as an adapter, we are using Pomelo, and pomelo is using MySqlConnector.

I do not really know the caller, but at the same time, I did not find another invocation than this:

// TryRevokeChainAsync
await foreach (var token in _tokenManager.FindByAuthorizationIdAsync(identifier))
{
    await _tokenManager.TryRevokeAsync(token);
}

I believe that the issue is caused by using AsAsyncEnumerable - inside FindByAuthorizationIdAsync which results in forbidden code:
(see https://mysqlconnector.net/troubleshooting/connection-reuse/)

using var command = new MySqlCommand("SELECT id FROM ...", connection);
using var reader = command.ExecuteReader();
while (reader.Read())
{
    var idToUpdate = reader.GetValue(0);
    connection.Execute("UPDATE ... SET ..."); // don't do this
}

Fix should be simple, do not use await foreach when we modify fetched data (we use https://www.nuget.org/packages/System.Linq.Async for that).
Or more complex, with introducing a flag passed to FindByAuthorizationIdAsync controlling if AsAsyncEnumerable or ToListAsync is used. This one is dirtier in my opinion but does not require third-party lib.

To reproduce

I will craft one if needed.

Exceptions (if any)

Here are exception details:

System.InvalidOperationException: POST /connect/token
An exception occurred while trying to revoke the token '<Guid>'.
This MySqlConnection is already in use. See https://fl.vu/mysql-conn-reuse

And stacktrace:

System.InvalidOperationException: This MySqlConnection is already in use. See https://fl.vu/mysql-conn-reuse
  File "/_/src/MySqlConnector/Core/ServerSession.cs", line 292, col 2, in void ServerSession.StartQuerying(ICancellableCommand command)
  File "/_/src/MySqlConnector/Core/CommandExecutor.cs", line 77, col 2, in async Task<MySqlDataReader> CommandExecutor.ExecuteReaderAsync(IReadOnlyList<IMySqlCommand> commands, ICommandPayloadCreator payloadCreator, CommandBehavior behavior, Activity activity, IOBehavior ioBehavior, CancellationToken cancellationToken)
  File "/_/src/MySqlConnector/MySqlCommand.cs", line 345, col 2, in async Task<MySqlDataReader> MySqlCommand.ExecuteReaderAsync(CommandBehavior behavior, IOBehavior ioBehavior, CancellationToken cancellationToken)
  File "/_/src/MySqlConnector/MySqlCommand.cs", line 337, col 3, in async Task<DbDataReader> MySqlCommand.ExecuteDbDataReaderAsync(CommandBehavior behavior, CancellationToken cancellationToken)
  File "RelationalCommand.cs", in async Task<RelationalDataReader> RelationalCommand.ExecuteReaderAsync(RelationalCommandParameterObject parameterObject, CancellationToken cancellationToken) x 2
  File "SingleQueryingEnumerable.cs", in async Task<bool> AsyncEnumerator.InitializeReaderAsync(AsyncEnumerator enumerator, CancellationToken cancellationToken)
  ?, in async Task<TResult> MySqlExecutionStrategy.ExecuteAsync<TState, TResult>(TState state, Func<DbContext, TState, CancellationToken, Task<TResult>> operation, Func<DbContext, TState, CancellationToken, Task<ExecutionResult<TResult>>> verifySucceeded, Cance...
  File "SingleQueryingEnumerable.cs", in async ValueTask<bool> AsyncEnumerator.MoveNextAsync()
  File "ShapedQueryCompilingExpressionVisitor.cs", in async Task<TSource> ShapedQueryCompilingExpressionVisitor.SingleOrDefaultAsync<TSource>(IAsyncEnumerable<TSource> asyncEnumerable, CancellationToken cancellationToken) x 2
  ?, in async ValueTask<TToken> OpenIddictEntityFrameworkCoreTokenStore<TToken, TApplication, TAuthorization, TContext, TKey>.FindByReferenceIdAsync(string identifier, CancellationToken cancellationToken)
  ?, in async IAsyncEnumerable<ValidationResult> OpenIddictTokenManager<TToken>.ValidateAsync(TToken token, CancellationToken cancellationToken)+MoveNext()
  ?, in async ValueTask OpenIddictTokenManager<TToken>.UpdateAsync(TToken token, CancellationToken cancellationToken)+GetValidationResultsAsync(?) x 2
  ?, in async ValueTask OpenIddictTokenManager<TToken>.UpdateAsync(TToken token, CancellationToken cancellationToken)
  ?, in async ValueTask<bool> OpenIddictTokenManager<TToken>.TryRevokeAsync(TToken token, CancellationToken cancellationToken)

[kevinchalet]

Hi,

Thanks for the report.

I may be hitting a race condition while revoking tokens in OpenIddictServerHandlers.Protection.cs:ValidateTokenEntry.HandleAsync.TryRevokeChainAsync.
We are using a MySql server, as an adapter, we are using Pomelo, and pomelo is using MySqlConnector.

It's not technically a race condition: await foreach is nothing more than a syntactic sugar for that:

var enumerator = _tokenManager.FindByAuthorizationIdAsync(identifier).GetAsyncEnumerator();
try
{
    while (await enumerator.MoveNextAsync())
    {
        await _tokenManager.TryRevokeAsync(enumerator.Current);
    }
}

finally
{
    await enumerator.DisposeAsync();
}

And as you can see, all the async calls are properly awaited: the enumerator moves to the next value in the sequence only when the token has been revoked.

What's likely happening here is that during the call to TryRevokeAsync, EF/Pomelo/MySqlConnector (or a combination of the 3 😄) end up reusing the connection created to iterate the results (still opened and waiting for the next call to MoveNextAsync() or DisposeAsync()) instead of creating a separate connection to revoke the tokens.

What's interesting is that I'm not aware of a similar issue with SQL Server. Maybe the SQL provider is smart enough to use different connections to handle that?

The only way to solve that - without introducing new one-shot manager/store methods (e.g something like IOpenIddictTokenManager/Store.(Try)RevokeByAuthorizationId()) - is to buffer the results before re-iterating the list of tokens to revoke. It's not ideal from a memory perspective but I don't see any other option.

In a future major version, introducing a IOpenIddictTokenManager/Store.(Try)RevokeByAuthorizationId() method with set-based deletes for the stores that support it will probably be necessary anyway. But it's a breaking change, so it will have to wait a bit 😄

[trejjam]

Hi, I did not mean it as race condition of c# or tasks themselves. But as a race condition of MySqlConnector which explicitly forbids it, because it did not finish reading Select * ... before the end of while/finally.

end up reusing the connection created

Yes, that's exactly what is happening. Every DbContext (for Pomelo) is using one MysqlConnection.

is to buffer the results before re-iterating

Yes, that's where ToListAsync (System.Linq.Async) comes in handy.

RevokeByAuthorizationId, method with set-based deletes for the stores that support it

Can you explain it more? I do not really understand what you mean by that. After understanding, I can prepare a PR on Monday.

---

There is also a workaround (use ToListAsync on results from FindByAuthorizationIdAsync in custom/extended OpenIddictTokenManager), where can I write it as documentation for Pomelo connector?

[kevinchalet]

Yes, that's exactly what is happening. Every DbContext (for Pomelo) is using one MysqlConnection.

That sounds like a bug/design issue to me. It's not clear why a second connection is not automatically used when the first one is busy. Don't they have a connection pool mechanism? 😕

Yes, that's where ToListAsync (System.Linq.Async) comes in handy.

The first OpenIddict 3.x previews depended on System.Linq.Async but we stopped using it for the reasons explained here: #980. That said, this method is so trivial to implement that we don't need RX for that 😄

openiddict-core/sandbox/OpenIddict.Sandbox.AspNetCore.Server/Helpers/AsyncEnumerableExtensions.cs

Lines 5 to 24 in 5e54e9b

	public static Task<List<T>> ToListAsync<T>(this IAsyncEnumerable<T> source)
	{
	if (source is null)
	{
	throw new ArgumentNullException(nameof(source));
	}
	return ExecuteAsync();
	async Task<List<T>> ExecuteAsync()
	{
	var list = new List<T>();
	await foreach (var element in source)
	{
	list.Add(element);
	}
	return list;
	}

(note: RX development pretty much halted, which would be a major concern if we decided to depend on it in future versions of OpenIddict)

Can you explain it more? I do not really understand what you mean by that. After understanding, I can prepare a PR on Monday.

It's the ability to generate DELETE FROM [...] WHERE [...] requests without having to do a SELECT to materialize the entities in EF's tracker and sending a DELETE request for each of the tracked entities.

Given that support for both set-based/bulk deletes and updates has been added to EF Core 7.0 (dotnet/efcore#795), I'll open a separate thread to track updating the PruneAsync() methods of the EF Core stores to use that on .NET 7.0+.

There is also a workaround (use ToListAsync on results from FindByAuthorizationIdAsync in custom/extended OpenIddictTokenManager), where can I write it as documentation for Pomelo connector?

While SQL Server remains the most popular option, MySQL+Pomelo is a combo I see quite often, so it likely deserves a built-in mitigation. I'll just patch the server event handler that uses this API to buffer the tokens before revoking them and consider it done 😅

[kevinchalet]

@trejjam workaround merged. Any chance you could give the 4.1.0 nightly builds a try to confirm it fixed your issue? Thanks.

[trejjam]

Sure thing, we have our workaround deployed from today:

I will let you know soon