Add missing awaits on CachingClientStore and CachingResourceStore

[joch0a]

What issue does this PR address?
Add missing awaits on CachingClientStore and CachingResourceStore

Does this PR introduce a breaking change?
no

Please check if the PR fulfills these requirements

• [x ] The commit follows our guidelines
• Unit Tests for the changes have been added (for bug fixes / features)

Other information:

[brockallen]

Aren't those the same, tho? And adding the async generates less efficient code, IIRC.

[joch0a]

On hard load we are getting this exception

Message: An unhandled exception occurred: Npgsql.NpgsqlException (0x80004005): The connection pool has been exhausted, either raise MaxPoolSize (currently **redacted**) or Timeout (currently 15 seconds)
   at Npgsql.ConnectorPool.AllocateLong(NpgsqlConnection conn, NpgsqlTimeout timeout, Boolean async, CancellationToken cancellationToken)
   at Npgsql.NpgsqlConnection.<>c__DisplayClass32_0.<<Open>g__OpenLong|0>d.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at Microsoft.EntityFrameworkCore.Storage.RelationalConnection.OpenDbConnectionAsync(Boolean errorsExpected, CancellationToken cancellationToken)
   at Microsoft.EntityFrameworkCore.Storage.RelationalConnection.OpenDbConnectionAsync(Boolean errorsExpected, CancellationToken cancellationToken)
   at Microsoft.EntityFrameworkCore.Storage.RelationalConnection.OpenAsync(CancellationToken cancellationToken, Boolean errorsExpected)
   at Microsoft.EntityFrameworkCore.Storage.RelationalCommand.ExecuteReaderAsync(RelationalCommandParameterObject parameterObject, CancellationToken cancellationToken)
   at Microsoft.EntityFrameworkCore.Query.Internal.QueryingEnumerable`1.AsyncEnumerator.InitializeReaderAsync(DbContext _, Boolean result, CancellationToken cancellationToken)
   at Npgsql.EntityFrameworkCore.PostgreSQL.Storage.Internal.NpgsqlExecutionStrategy.ExecuteAsync[TState,TResult](TState state, Func`4 operation, Func`4 verifySucceeded, CancellationToken cancellationToken)
   at Microsoft.EntityFrameworkCore.Query.Internal.QueryingEnumerable`1.AsyncEnumerator.MoveNextAsync()
   at Microsoft.EntityFrameworkCore.Query.ShapedQueryCompilingExpressionVisitor.SingleOrDefaultAsync[TSource](IAsyncEnumerable`1 asyncEnumerable, CancellationToken cancellationToken)
   at Microsoft.EntityFrameworkCore.Query.ShapedQueryCompilingExpressionVisitor.SingleOrDefaultAsync[TSource](IAsyncEnumerable`1 asyncEnumerable, CancellationToken cancellationToken)
   at IdentityServer4.EntityFramework.Stores.ClientStore.FindClientByIdAsync(String clientId)
   at IdentityServer4.Stores.ValidatingClientStore`1.FindClientByIdAsync(String clientId)
   at IdentityServer4.Extensions.ICacheExtensions.GetAsync[T](ICache`1 cache, String key, TimeSpan duration, Func`1 get, ILogger logger)
   at IdentityServer4.Stores.CachingClientStore`1.FindClientByIdAsync(String clientId)

-------------------------
   at IdentityServer4.Stores.IClientStoreExtensions.FindEnabledClientByIdAsync(IClientStore store, String clientId) <<<<<<<<<<< Here
-------------------------

   at IdentityServer4.Validation.ClientSecretValidator.ValidateAsync(HttpContext context)
   at IdentityServer4.Endpoints.TokenEndpoint.ProcessTokenRequestAsync(HttpContext context)
   at IdentityServer4.Endpoints.TokenEndpoint.ProcessAsync(HttpContext context)
   at IdentityServer4.Hosting.IdentityServerMiddleware.Invoke(HttpContext context, IEndpointRouter router, IUserSession session, IEventService events)
   at IdentityServer4.Hosting.IdentityServerMiddleware.Invoke(HttpContext context, IEndpointRouter router, IUserSession session, IEventService events)
   at IdentityServer4.Hosting.MutualTlsTokenEndpointMiddleware.Invoke(HttpContext context, IAuthenticationSchemeProvider schemes)
   at Microsoft.AspNetCore.Authentication.AuthenticationMiddleware.Invoke(HttpContext context)
   at IdentityServer4.Hosting.BaseUrlMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Diagnostics.StatusCodePagesMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.ResponseCompression.ResponseCompressionMiddleware.Invoke(HttpContext context)
   at IdentityProvider.Api.Services.ExceptionMiddleware.InvokeAsync(HttpContext httpContext) in ****\ExceptionMiddleware.cs:line 37

Checking out the code, we figure it out that the FindClientByIdAsync on the CachingClientStore is not being awaited and it is the only place on the entire project that is not being awaited and it is where the exception is being raised.
I feel like without the async the connection with the DB is not being opened/closed as expected.

[brockallen]

Do you have any more insight into this? The fix you propose fixes the issue in postgres?

[brockallen]

Also, the change you propose -- doesn't that always trigger loading from the DB? Whereas the point of the Func is to defer the DB lookup unless it's needed.

[joch0a]

Do you have any more insight into this? The fix you propose fixes the issue in postgres?

Yes, this change solves the issue in Postgres, I haven't tested any other RDBMS but I think they should have similar behavior.
To reproduce it locally

1. I reduce the MaxPoolSize around 30 to get the error faster.
2. I run a script on Postman that basically makes 10.000 token requests.

Also, the change you propose -- doesn't that always trigger loading from the DB? Whereas the point of the Func is to defer the DB lookup unless it's needed.
Hmmm I think that the code is just saying that the Func< Task > should await the asynchronous method call, why would it trigger loading from the DB?

I would like to share a screen recording or at least the script that I have on Postman, but I think that this cannot be possible at least on Github. If you want I can share you that through email or something like that.

Thanks!

[brockallen]

Ok I just tested it and it doesn't seem to have any negative impact. I think we have other places like this doing caching in the code, so perhaps you can check for others an update this PR?

I just checked, it looks like CachingResourceStore is the only other place. Can you do the same for that one as well, please? Thanks

[dnfadmin]

All CLA requirements met.

[joch0a]

Done!
I've checked the code trying to find another place where a Func<< Task>> is not using the await and I have found this.

DeviceAuthorizationResponseGeneratorTests.cs

public void ProcessAsync_when_valiationresult_null_exect_exception()
{
    Func<Task> act = () => generator.ProcessAsync(null, TestBaseUrl);
    act.Should().Throw<ArgumentNullException>();
    ...
}

AuthorizeInteractionResponseGeneratorTests_Consent.cs

[Fact]
public void ProcessConsentAsync_NullRequest_Throws()
{
    Func<Task> act = () => _subject.ProcessConsentAsync(null, new ConsentResponse());
    .
    .
}


IResourceStoreExtensionsTests.cs

[Fact]
public void GetAllEnabledResourcesAsync_on_duplicate_identity_scopes_should_fail()
            new IdentityResource { Name = "A" } }
    };

    Func<Task> a = () => store.GetAllEnabledResourcesAsync();
    ....
}

TokenRequestValidation_General_Invalid.cs

{
	..
	Func<Task> act = () => validator.ValidateRequestAsync(null, null);

    act.Should().Throw<ArgumentNullException>();
    parameters.Add(OidcConstants.TokenRequest.RedirectUri, "https://server/cb");

    Func<Task> act = () => validator.ValidateRequestAsync(parameters, null);
    ..
}
            

I haven't changed it on the PR because I did not take the time to see how the tests were implemented, but if you wanted I can do the same on those classes

[brockallen]

Thanks

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.