Solving OnSendingHeaders deadlock with Katana OpenID Connect Middleware

Question

I'm trying to make use of the OpenID Connect authentication middleware provided by the Katana project.

There's a bug in the implementation which causes a deadlock under these conditions:

1. Running in a host where the request has thread-affinity (e.g. IIS).
2. The OpenID Connect metadata document has not been retrieved or the cached copy has expired.
3. The application calls SignOut for the authentication method.
4. An action happens in the application which causes a write to the response stream.

The deadlock happens due to the way the authentication middleware handles the callback from the host signalling that headers are being sent. The root of the problem is in this method:

private static void OnSendingHeaderCallback(object state)
{
    AuthenticationHandler handler = (AuthenticationHandler)state;
    handler.ApplyResponseAsync().Wait();
}

From Microsoft.Owin.Security.Infrastructure.AuthenticationHandler

The call to Task.Wait() is only safe when the returned Task has already completed, which it has not done in the case of the OpenID Connect middleware.

The middleware uses an instance of Microsoft.IdentityModel.Protocols.ConfigurationManager<T> to manage a cached copy of its configuration. This is an asychnronous implementation using SemaphoreSlim as an asynchronous lock and an HTTP document retriever to obtain the configuration. I suspect this to be the trigger of the deadlock Wait() call.

This is the method I suspect to be the cause:

public async Task<T> GetConfigurationAsync(CancellationToken cancel)
{
    DateTimeOffset now = DateTimeOffset.UtcNow;
    if (_currentConfiguration != null && _syncAfter > now)
    {
        return _currentConfiguration;
    }

    await _refreshLock.WaitAsync(cancel);
    try
    {
        Exception retrieveEx = null;
        if (_syncAfter <= now)
        {
            try
            {
                // Don't use the individual CT here, this is a shared operation that shouldn't be affected by an individual's cancellation.
                // The transport should have it's own timeouts, etc..

                _currentConfiguration = await _configRetriever.GetConfigurationAsync(_metadataAddress, _docRetriever, CancellationToken.None);
                Contract.Assert(_currentConfiguration != null);
                _lastRefresh = now;
                _syncAfter = DateTimeUtil.Add(now.UtcDateTime, _automaticRefreshInterval);
            }
            catch (Exception ex)
            {
                retrieveEx = ex;
                _syncAfter = DateTimeUtil.Add(now.UtcDateTime, _automaticRefreshInterval < _refreshInterval ? _automaticRefreshInterval : _refreshInterval);
            }
        }

        if (_currentConfiguration == null)
        {
            throw new InvalidOperationException(string.Format(CultureInfo.InvariantCulture, ErrorMessages.IDX10803, _metadataAddress ?? "null"), retrieveEx);
        }

        // Stale metadata is better than no metadata
        return _currentConfiguration;
    }
    finally
    {
        _refreshLock.Release();
    }
}

I have tried adding .ConfigureAwait(false) to all of the awaited operations in an effort to marshal continuations onto the thread pool, rather than the ASP.NET worker thread, but I've not had any success in avoiding the deadlock.

Is there a deeper issue I can tackle? I don't mind replacing components - I have already created my own experimental implementations of IConfiguratioManager<T>. Is there a simple fix that can be applied to prevent the deadlock?

Answer 1

@Tragedian We took these fixes for this issue. Can you update and see if the issue still exists (we thought we had it fixed with 184, but as you see we had 185). Another customer has had success with the latest nuget.

http://www.nuget.org/packages/Microsoft.IdentityModel.Protocol.Extensions/1.0.2.206221351

https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/185/files

https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/184/files

Answer 2

I can't comment on the accepted answer, but even with that specific nuget the problem seems to persist for me :/

I've found that I need to modify the ConfigurationManager#GetConfigurationAsync lines:

await _refreshLock.WaitAsync(cancel);

to

_refreshLock.Wait(cancel);

and

_currentConfiguration = await _configRetriever.GetConfigurationAsync(_metadataAddress, _docRetriever, CancellationToken.None)

to

_currentConfiguration =  _configRetriever.GetConfigurationAsync(_metadataAddress, _docRetriever, CancellationToken.None).Result;

Or alternatively I put a ConfigureAwait(false) on both of the calls and wrap the 'GetConfigurationAsync' in another method that blocks with a '.Result' call and returns it in a new already completed task.

If I do this then the deadlocks on logout no longer occur for me for more than 1 user (the previous fix addressed a single user logging out.)

However clearly this is making the 'GetConfigurationAsync' method decidedly synchronous :/