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:

Running in a host where the request has thread-affinity (e.g. IIS).
The OpenID Connect metadata document has not been retrieved or the cached copy has expired.
The application calls SignOut for the authentication method.
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?

I don't hope this will change anything, but try `GetAwaiter().GetResult()` instead of `Wait()`. — Paulo Morgado, Jul 06 '15 at 12:55
@PauloMorgado Unfortunately, the `OnSendingHeaders` code is buried deep in the `AuthenticationHandler` class and cannot be altered. — Paul Turner, Jul 06 '15 at 12:57
Alternatively, try waiting for `Task.IsComplete` to be true. Use [SpinWait](https://msdn.microsoft.com/library/system.threading.spinwait.aspx "SpinWait Structure") to wait without switching context. — Paulo Morgado, Jul 06 '15 at 12:57
No, it's defined in `Microsoft.Owin.Security.Infrastructure.AuthenticationHandler`. — Paul Turner, Jul 06 '15 at 13:02
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? — Nitram, Jul 06 '15 at 14:51
@Nitram Yes, I have specifically mentioned: IIS is the host, so `Wait()` deadlocks because of the ASP.NET synchronisation context. — Paul Turner, Jul 06 '15 at 14:54
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`. — Nitram, Jul 06 '15 at 14:57

score 3 · Accepted Answer · answered Jul 06 '15 at 19:28

3

@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

answered Jul 06 '15 at 19:28

Brent Schmaltz

1,151
6
7

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. – Paul Turner Jul 14 '15 at 14:33
What was your test scenario? How did you test it properly? – wałdis iljuczonok Aug 23 '16 at 15:43

ciaranj · Answer 2 · 2016-02-24T22:11:28.093

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

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. — wałdis iljuczonok, Aug 26 '16 at 06:53

Solving OnSendingHeaders deadlock with Katana OpenID Connect Middleware

2 Answers2