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?

Periwinkle

I don't hope this will change anything, but try GetAwaiter().GetResult() instead of Wait(). – Railey

@PauloMorgado Unfortunately, the OnSendingHeaders code is buried deep in the AuthenticationHandler class and cannot be altered. – Periwinkle

Alternatively, try waiting for Task.IsComplete to be true. Use SpinWait to wait without switching context. – Railey

Can you change OnSendingHeaderCallback? – Railey

No, it's defined in Microsoft.Owin.Security.Infrastructure.AuthenticationHandler. – Periwinkle

This deadlock from mixing await and wait should only happen if you are in a thread with a SynchronizationContext that syncs back to a execution queue. Is there any special SynchronizationContext present in your case? – Admittedly

@Admittedly Yes, I have specifically mentioned: IIS is the host, so Wait() deadlocks because of the ASP.NET synchronisation context. – Periwinkle

Do you have a chance to push in another layer that does a Wait based wait on a new background Task? So you can run GetConfigurationAsync outside of the SynchronizationContext? The only other thing I can think if is to get rid of the await and do it all with Wait. – Admittedly

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

Peremptory

I have only had an opportunity to test briefly. It passed my initial test scenario, but I need to do more to verify I can no longer create a deadlock. I will accept this answer when I get the opportunity to verify the fix properly. – Periwinkle

What was your test scenario? How did you test it properly? – Lizzettelizzie

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 :/

Ringtail

With your proposed code changes I still got deadlock. What I did was - changed to _refreshLock.Wait(cancel); and await _configRetriever.GetConfigurationAsync(...).ConfigureAwait(false);. This is preventing me for getting deadlock, however - not sure that code is correct. – Lizzettelizzie