Microsoft-identity-web: [Bug] Adding Redis as Distributed Cache Triggers NullReferenceException

Created on 29 Apr 2020  路  32Comments  路  Source: AzureAD/microsoft-identity-web

Which Version of Microsoft Identity Web are you using ?
0.1.1 preview

Where is the issue?

  • Web App

    • [ ] Sign-in users

    • [ ] Sign-in users and call web APIs

  • Web API

    • [ ] Protected web APIs (Validating tokens)

    • [ ] Protected web APIs (Validating scopes)

    • [ ] Protected web APIs call downstream web APIs

  • Token cache serialization

    • [ ] In Memory caches

    • [ ] Session caches

    • [X] Distributed caches

Other? - please describe;

Is this a new or existing app?
Transitioning and existing application from the V1 endpoints to V2 and using this library. The conversion is just about complete but cannot finish testing with this bug in place.

Repro
Simply adding Redis distributed cache to the tail of the services collection causes a NullReferenceException when attempting to fetch access token. Apparently, for some reason, this causes the HttpContextAccessor to return a null.

        public static IServiceCollection AddDistributedOrLocalMemoryCache(this IServiceCollection services, IConfiguration configuration) {
            services.AddDistributedTokenCaches();

            services.AddDistributedRedisCache((RedisCacheOptions redisOptions) => {
                redisOptions.Configuration = configuration.RedisConnectionString();
                redisOptions.InstanceName = $"fischer-web-app-{configuration.EnvironmentName().ToLower()}: ";
            });

            return services;
        }

Expected behavior
No exception

Actual behavior
A NullReferenceException is thrown

Possible Solution
Still investigating

Additional context/ Logs / Screenshots
System.NullReferenceException
HResult=0x80004003
Message=Object reference not set to an instance of an object.
Source=Microsoft.Identity.Web
StackTrace:
at Microsoft.Identity.Web.HttpContextExtensions.GetTokenUsedToCallWebAPI(HttpContext httpContext) in D:\a\1\s\src\Microsoft.Identity.Web\HttpContextExtensions.cs:line 29
at Microsoft.Identity.Web.TokenCacheProviders.MsalAbstractTokenCacheProvider.GetCacheKey(Boolean isAppTokenCache) in D:\a\1\s\src\Microsoft.Identity.Web\TokenCacheProviders\MsalAbstractTokenCacheProvider.cs:line 66
at Microsoft.Identity.Web.TokenCacheProviders.MsalAbstractTokenCacheProvider.d__6.MoveNext() in D:\a\1\s\src\Microsoft.Identity.Web\TokenCacheProviders\MsalAbstractTokenCacheProvider.cs:line 94
at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw() in /_/src/System.Private.CoreLib/shared/System/Runtime/ExceptionServices/ExceptionDispatchInfo.cs:line 63
at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) in /_/src/System.Private.CoreLib/shared/System/Runtime/CompilerServices/TaskAwaiter.cs:line 180
at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) in /_/src/System.Private.CoreLib/shared/System/Runtime/CompilerServices/TaskAwaiter.cs:line 151
at System.Runtime.CompilerServices.ConfiguredTaskAwaitable.ConfiguredTaskAwaiter.GetResult() in /_/src/System.Private.CoreLib/shared/System/Runtime/CompilerServices/TaskAwaiter.cs:line 474
at Microsoft.Identity.Client.TokenCache.d__104.MoveNext() in D:\a\1\s\src\client\Microsoft.Identity.Client\TokenCache.Notifications.cs:line 47
at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw() in /_/src/System.Private.CoreLib/shared/System/Runtime/ExceptionServices/ExceptionDispatchInfo.cs:line 63
at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) in /_/src/System.Private.CoreLib/shared/System/Runtime/CompilerServices/TaskAwaiter.cs:line 180
at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) in /_/src/System.Private.CoreLib/shared/System/Runtime/CompilerServices/TaskAwaiter.cs:line 151
at System.Runtime.CompilerServices.ConfiguredTaskAwaitable.ConfiguredTaskAwaiter.GetResult() in /_/src/System.Private.CoreLib/shared/System/Runtime/CompilerServices/TaskAwaiter.cs:line 474
at Microsoft.Identity.Client.Cache.CacheSessionManager.d__13.MoveNext() in D:\a\1\s\src\client\Microsoft.Identity.Client\Cache\CacheSessionManager.cs:line 115
at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw() in /_/src/System.Private.CoreLib/shared/System/Runtime/ExceptionServices/ExceptionDispatchInfo.cs:line 63
at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) in /_/src/System.Private.CoreLib/shared/System/Runtime/CompilerServices/TaskAwaiter.cs:line 180
at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) in /_/src/System.Private.CoreLib/shared/System/Runtime/CompilerServices/TaskAwaiter.cs:line 151
at System.Runtime.CompilerServices.ConfiguredTaskAwaitable.ConfiguredTaskAwaiter.GetResult() in /_/src/System.Private.CoreLib/shared/System/Runtime/CompilerServices/TaskAwaiter.cs:line 474
at Microsoft.Identity.Client.Cache.CacheSessionManager.d__12.MoveNext() in D:\a\1\s\src\client\Microsoft.Identity.Client\Cache\CacheSessionManager.cs:line 83
at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw() in /_/src/System.Private.CoreLib/shared/System/Runtime/ExceptionServices/ExceptionDispatchInfo.cs:line 63
at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) in /_/src/System.Private.CoreLib/shared/System/Runtime/CompilerServices/TaskAwaiter.cs:line 180
at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) in /_/src/System.Private.CoreLib/shared/System/Runtime/CompilerServices/TaskAwaiter.cs:line 151
at System.Runtime.CompilerServices.ConfiguredTaskAwaitable1.ConfiguredTaskAwaiter.GetResult() in /_/src/System.Private.CoreLib/shared/System/Runtime/CompilerServices/TaskAwaiter.cs:line 563 at Microsoft.Identity.Client.ClientApplicationBase.<GetAccountsAsync>d__14.MoveNext() in D:\a\1\s\src\client\Microsoft.Identity.Client\ClientApplicationBase.cs:line 113 at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw() in /_/src/System.Private.CoreLib/shared/System/Runtime/ExceptionServices/ExceptionDispatchInfo.cs:line 63 at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) in /_/src/System.Private.CoreLib/shared/System/Runtime/CompilerServices/TaskAwaiter.cs:line 180 at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) in /_/src/System.Private.CoreLib/shared/System/Runtime/CompilerServices/TaskAwaiter.cs:line 151 at System.Runtime.CompilerServices.ConfiguredTaskAwaitable1.ConfiguredTaskAwaiter.GetResult() in /_/src/System.Private.CoreLib/shared/System/Runtime/CompilerServices/TaskAwaiter.cs:line 563
at Microsoft.Identity.Web.TokenAcquisition.d__17.MoveNext() in D:\a\1\s\src\Microsoft.Identity.Web\TokenAcquisition.cs:line 410
at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw() in /_/src/System.Private.CoreLib/shared/System/Runtime/ExceptionServices/ExceptionDispatchInfo.cs:line 63
at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) in /_/src/System.Private.CoreLib/shared/System/Runtime/CompilerServices/TaskAwaiter.cs:line 180
at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) in /_/src/System.Private.CoreLib/shared/System/Runtime/CompilerServices/TaskAwaiter.cs:line 151
at System.Runtime.CompilerServices.ConfiguredTaskAwaitable1.ConfiguredTaskAwaiter.GetResult() in /_/src/System.Private.CoreLib/shared/System/Runtime/CompilerServices/TaskAwaiter.cs:line 563 at Microsoft.Identity.Web.TokenAcquisition.<GetAccessTokenForUserAsync>d__12.MoveNext() in D:\a\1\s\src\Microsoft.Identity.Web\TokenAcquisition.cs:line 203 at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw() in /_/src/System.Private.CoreLib/shared/System/Runtime/ExceptionServices/ExceptionDispatchInfo.cs:line 63 at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) in /_/src/System.Private.CoreLib/shared/System/Runtime/CompilerServices/TaskAwaiter.cs:line 180 at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) in /_/src/System.Private.CoreLib/shared/System/Runtime/CompilerServices/TaskAwaiter.cs:line 151 at System.Runtime.CompilerServices.TaskAwaiter1.GetResult() in /_/src/System.Private.CoreLib/shared/System/Runtime/CompilerServices/TaskAwaiter.cs:line 369
at WebApp.Startup.d__6.MoveNext() in G:\acquisitions.visualstudio.com\acquisition-web-app-2\src\Startup.cs:line 98
at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw() in /_/src/System.Private.CoreLib/shared/System/Runtime/ExceptionServices/ExceptionDispatchInfo.cs:line 63
at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) in /_/src/System.Private.CoreLib/shared/System/Runtime/CompilerServices/TaskAwaiter.cs:line 180
at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) in /_/src/System.Private.CoreLib/shared/System/Runtime/CompilerServices/TaskAwaiter.cs:line 151
at System.Runtime.CompilerServices.TaskAwaiter.GetResult() in /_/src/System.Private.CoreLib/shared/System/Runtime/CompilerServices/TaskAwaiter.cs:line 107
at WebApp.Startup.<>c__DisplayClass5_0.<b__0>d.MoveNext() in G:\acquisitions.visualstudio.com\acquisition-web-app-2\src\Startup.cs:line 66
at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw() in /_/src/System.Private.CoreLib/shared/System/Runtime/ExceptionServices/ExceptionDispatchInfo.cs:line 63
at System.Threading.Tasks.Task.<>c.b__139_1(Object state) in /_/src/System.Private.CoreLib/shared/System/Threading/Tasks/Task.cs:line 1954
at System.Threading.QueueUserWorkItemCallback.<>c.<.cctor>b__6_0(QueueUserWorkItemCallback quwi) in /_/src/System.Private.CoreLib/shared/System/Threading/ThreadPool.cs:line 808
at System.Threading.ExecutionContext.RunForThreadPoolUnsafeTState in /_/src/System.Private.CoreLib/shared/System/Threading/ExecutionContext.cs:line 335
at System.Threading.QueueUserWorkItemCallback.Execute() in /_/src/System.Private.CoreLib/shared/System/Threading/ThreadPool.cs:line 824
at System.Threading.ThreadPoolWorkQueue.Dispatch() in /_/src/System.Private.CoreLib/shared/System/Threading/ThreadPool.cs:line 677
at System.Threading._ThreadPoolWaitCallback.PerformWaitCallback() in /_/src/System.Private.CoreLib/src/System/Threading/ThreadPool.CoreCLR.cs:line 29

external investigate token cache serialization - distributed cache
Source
picture strisys

All 32 comments

Will anyone have a chance to look at this soon?

strisys picture strisys on 6 May 2020

@strisys : did you read this?
https://docs.microsoft.com/en-us/aspnet/core/performance/caching/distributed?view=aspnetcore-3.1#distributed-redis-cache

jmprieur picture jmprieur on 8 May 2020

Thank you Jean-Marc

I replaced Microsoft.Extensions.Caching.Redis with Microsoft.Extensions.Caching.StackExchangeRedis.

Unfortunately I still cannot reliably get a token when using the ITokenAcquisition.GetAccessTokenForAppAsync member. I do have a work-around and that is to use the synchronous members of the TokenCache in MsalAbstractTokenCacheProvider as follows. 

    public Task InitializeAsync(ITokenCache tokenCache)
    {
        ...

        tokenCache.SetBeforeAccess(OnBeforeAccess);
        tokenCache.SetAfterAccess(OnAfterAccess);
        tokenCache.SetBeforeWrite(OnBeforeWrite);

        // tokenCache.SetBeforeAccessAsync(OnBeforeAccessAsync);
        // tokenCache.SetAfterAccessAsync(OnAfterAccessAsync);
        // tokenCache.SetBeforeWriteAsync(OnBeforeWriteAsync);

        return Task.CompletedTask;
    }

It is possible the original context is lost causing the getter of IHttpContextAccessor to return a null HttpContext?

strisys picture strisys on 11 May 2020

@strisys : that's a good question. It's totally possible that the IHttpContext is lost. I'll have a look at what we can do
cc: @bgavrilMS @jennyf19 @pmaytak @henrik-me

jmprieur picture jmprieur on 12 May 2020

Ack @jmprieur , let me know if you need help here. I see a lot of GetAwaiter().GetResult() in the stack trace, which potentially defeat the purpose of async.

bgavrilMS picture bgavrilMS on 12 May 2020

Could the issue be in the TokenCache? For example ...

    ... 
    async Task ITokenCacheInternal.OnBeforeAccessAsync(TokenCacheNotificationArgs args)
    {
        BeforeAccess?.Invoke(args);
        if (AsyncBeforeAccess != null)
        {
            await AsyncBeforeAccess
                .Invoke(args)
                .ConfigureAwait(false);
        }
    }
    ...

In other words, should ConfigureAwait(false) be used here?

strisys picture strisys on 12 May 2020

Hmm, thanks for pointing this out @strisys. This SO post also confirms that the context is lost after the .ConfigureAwait.

From a logical perspective, I think MSAL is doing the right thing though. MSAL does not make any assumption about the context - as it could be ASP.NET Core, WinForms, WPF, UWP etc.

However, Identity.Web cache implementations rely on the context (via HttpContext). So I think they should use .ConfigureAwait(true) to ensure the context is preserved. @jmprieur - do you want me to try this out?

PS: long but well explained article on ConfigureAwait

if you鈥檙e writing general-purpose library code, use ConfigureAwait(false). - this is MSAL
Identity.Web is not general purpose as it relies on HttpContext.

bgavrilMS picture bgavrilMS on 12 May 2020

I guess I am somewhat confused. If TokenCache (MSAL) is using .ConfigureAwait(false) isn't it assuming its OK for the context to be lost? If so, should it be making that assumption?

strisys picture strisys on 12 May 2020

Yes, but as that article points out, libraries should assume it is ok for context to be lost. Keeping the context leads to perf degradation.

What I'm trying to get to is that the fix should go into Identity.Web and not into MSAL. Because if we use .ConfigureAwait(true) in MSAL, it would degrade perf in all token cache implementations, including those that are not context sensitive.

Also from a tactical perspective - making this change in Identity.Web is straight forward. But making such a change in MSAL will require some analysis of the impact / perf etc.

bgavrilMS picture bgavrilMS on 12 May 2020

OK. The perf issue makes perfect sense. MSAL could give the option to preserve the context but I don't think I have ever seen that done.

I guess I am curious as to how the context is going to be preserved by Identity.Web if MSAL is stripping it off. There must be a simple solution I am not seeing.

strisys picture strisys on 12 May 2020

I'm hoping that if we change the ConfigureAwait(false) calls from here to ConfigureAwait(true) it will do the trick.

bgavrilMS picture bgavrilMS on 12 May 2020

@strisys - does the bug occur every time?

bgavrilMS picture bgavrilMS on 12 May 2020

@bgavrilMS : what about

https://github.com/AzureAD/microsoft-identity-web/blob/09ee45a9708566290621a44e31987e562ff07f84/src/Microsoft.Identity.Web/TokenCacheProviders/Distributed/MsalDistributedTokenCacheAdapter.cs#L64 ?

and

https://github.com/AzureAD/microsoft-identity-web/blob/09ee45a9708566290621a44e31987e562ff07f84/src/Microsoft.Identity.Web/TokenCacheProviders/Distributed/MsalDistributedTokenCacheAdapter.cs#L74

jmprieur picture jmprieur on 12 May 2020

Those too ... code that is context sensitive (in your case HttpContext) should not use ConfigureAwait(false)

bgavrilMS picture bgavrilMS on 12 May 2020
👍1

@bgavrilMS. The NullReferenceException is very consistent once I swapped in my real LOB javascript libraries in from a basic page I was using to test things out. That makes sense I suppose since my basic page was not making multiple AJAX calls at the same time. Once more threads are in play I think the problem really surfaced.

I am not using ConfigureAwait(false) when I make the call to ITokenAcquisition.GetAccessTokenForAppAsync.

strisys picture strisys on 12 May 2020

Sorry guys but it looks like I misspoke. My workaround does not seem to be working. I must have been doing something differently yesterday.

I put in checks in all the members of MsalDistributedTokenCacheAdapter to make sure I could still get the HttpContext before calling in IDistributedCache. It seems to be the case that the HttpContext is still intact via the IHttpContextAccessor. I also used the synchronous members of the IDistributedCache. Here is an example of the changes I made in MsalDistributedTokenCacheAdapter purely for debugging purposes.

    protected override async Task<byte[]> ReadCacheBytesAsync(string cacheKey)
    {
        if (_httpContextAccessor.HttpContext == null) {
            throw new InvalidOperationException($"Failed to obtain {typeof(HttpContext)} from {_httpContextAccessor.GetType()}");
        }

        return await Task.Run(() => {
            return _distributedCache.Get(cacheKey);
        }).ConfigureAwait(true);

        // return await _distributedCache.GetAsync(cacheKey).ConfigureAwait(true);
    }

Regardless, there are still asynchronous calls in the stack trace that use GetAwaiter(false) even though I am using synchronous callback methods of TokenCache. One such call is from CacheSessionManager.RefreshCacheForReadOperationsAsync to TokenCache.OnBeforeAccessAsync which in turn calls the callback TokenCache.BeforeAccess. Its possible the context is lost at this point.

I really don't see a workaround for this. It seems like HttpContext is going to have to be explicitly passed down the callstack. Any thoughts?

strisys picture strisys on 12 May 2020

We may have to fix this in MSAL. One way of fixing is to pass along a SyncronizationContext in the cache callbacks. Then token cache implementations could use it to marshall back the calls to the correct thread. Needs some experimenting...

bgavrilMS picture bgavrilMS on 13 May 2020

Seems to make sense.

Would a decorator with the threading behavior necessary to preserve the context make sense for now in TokenAcquisition.BuildConfidentialClientApplicationAsync.

  ....
  ITokenCache tokenCache = new ContextPreservingTokenCache(app.AppTokenCache);
  await _tokenCacheProvider.InitializeAsync(tokenCache).ConfigureAwait(false);
  ....
strisys picture strisys on 13 May 2020

@pmaytak - I'm caught up with some MSAL work at the moment. Do you have time to work on this? I'm happy to help and get any MSAL changes in of course.

bgavrilMS picture bgavrilMS on 13 May 2020

I can take a look tomorrow to see if I can get it to work.

strisys picture strisys on 13 May 2020

@bgavrilMS I can make time for this.

pmaytak picture pmaytak on 13 May 2020

Are there instructions on building MSAL? I had some issues so I could not try anything out.

strisys picture strisys on 15 May 2020

@strisys here are directions on building MSAL. you can comment out the parts on building Android/iOS, as it will save you time.

jennyf19 picture jennyf19 on 15 May 2020

Thank you @jennyf19

strisys picture strisys on 15 May 2020

For this scenario, as Jenny said, you can comment out lines 8-11 from https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/blob/59546f400b05b3a5e29cb6e825b04e21a2548554/src/client/Directory.Build.props#L8

And then Microsoft.Identity.Client should build. Just unload any projects that don't build, there are a ton of Xamarin apps in the solution.

bgavrilMS picture bgavrilMS on 15 May 2020

I was able to get the MSAL library to build and incorporate it but my sense is its not the issue. I think there is a race condition which is why this has been so elusive. Somehow it seems like the HttpContext stored in an AsyncLocal<T> of HttpContextAccessor is being nullified under certain conditions (faults). I do not have a full understanding on what those are just yet. Nonetheless, the code _may_ have to handle this condition more gracefully.

strisys picture strisys on 16 May 2020

I was able to find the race condition in my middleware and will see if I can reproduce the NullReferenceException going forward. So far so good

strisys picture strisys on 19 May 2020
👍1

Thanks for the update @strisys

bgavrilMS picture bgavrilMS on 19 May 2020

@strisys Thanks for the update. I did try and was unable to reproduce the broken behavior with distributed Redis cache. I did look at how AsyncLocal works and it does seem that if the context is switched, the HttpContext would be null. Looking forward to your findings.

pmaytak picture pmaytak on 20 May 2020

So far so good as I have not seen the issue since the race condition was corrected.

strisys picture strisys on 21 May 2020
👍1

Thanks for the updates @strisys

jennyf19 picture jennyf19 on 21 May 2020

@strisys : proposing to close this issue since this is working for you now.
Feel free to reopen.

jmprieur picture jmprieur on 22 May 2020
👍1
Was this page helpful?
0 / 5 - 0 ratings

Related issues

glasody picture glasody  路  5Comments
mgmccarthy picture mgmccarthy  路  4Comments
CalamityLorenzo picture CalamityLorenzo  路  3Comments
jennyf19 picture jennyf19  路  5Comments
durandt picture durandt  路  3Comments