Microsoft-authentication-library-for-dotnet: Async versions of BeforeAccess and AfterAccess

[dmcweeney]

Hi,

Any plans to provide async versions of the BeforeAccess and AfterAccess notifications?

Thanks

Donal

[jmprieur]

@dmcweeney can you please explain your scenario?

[dmcweeney]

Hi @jmprieur , sorry for the delay in getting back.
Looking at the code I see in the async methods that require access to TokenCache it calls the BeforeAccess and AfterAccess synchronously.
In my case, I then was calling the IDistributedCache async methods to get/set the token values in the redis cache up on Azure. And after a few calls my web app deadlocked.
I used the .GetAwaiter().GetResult() on these calls which can result in deadlocks, and there was lots of other recommendations out these on using Task.Run etc.

However the most frequent recommendation was to re-evaluate the code, and that the async methods should be calling async methods through the lifetime of the operation.

Thus my request! Does that explain the scenario ok?

Thanks

Donal

[jmprieur]

Thanks @dmcweeney for this explanation. It's better for us to understand the scenario (you get deadlocks)

[AlexandreArpin]

Gonna ride on this, it's a frequent use case to persist the Token Cache in a distributed manner, it'd be nice to have a TokenCache implementation that uses Async signature.

Now that you decorate the Token Cache instead of inheriting from it, it's a bit less annoying but those BeforeAccess/AfterAccess could really use having an Async signature since most of the time the implementation are gonna be doing IO of some sort.

[juunas11]

👍 for this. We should not have to do blocking calls on async APIs.

[henrik-me]

Discussed this during triage. Seems like this will benefit webapps the most, thus prioritizing for v3. Not blocking a specific v3 release but should be in an update soon after if not included.

[bgavrilMS]

Not needed for 3.0.4 - we need to focus on features and fixes that affect the public API only so that we can remove the -preview tag.

[juunas11]

I actually tried to modify the library towards async but it is quite significant work. Hope you find a good library for doing re-entrant async locking as the current code in TokenCache would require one.

[MarkZuber]

A question as I'm researching this. It looks like the main reason for re-entrancy is that folks would need to call Serialize or Deserialize on the cache during the callbacks and since the async callback would be on a different thread, we'll hit lock re-entrance problems trying to (de)serialize the cache data.

If we added new async callbacks, we could have the developer specify in the registration what type of serialization they want (e.g. MSALv3, AdalV3, etc) and the NotificationArgs class could contain the deserialized value of the current cache (in the case of OnAfterAccess) and a place to set the serialized value of the cache (in the case of OnBeforeAccess). Alternatively, we could have multiple properties in the notification args with the deserialized values and/or place to set the new serialized values from storage. There's a bit more of a performance hit since we may do extra serialization on the AfterAccess calls to send the different formats but that may be OK given the usual sizes of the cache. But the advantage here would be simplification of the API.

Are folks using any other tokencache or account API calls (that may hit the cache indirectly) in these callbacks?

[AlexandreArpin]

It looks like the main reason for re-entrancy is that folks would need to call Serialize or Deserialize on the cache during the callbacks

That is our use case yes. We rely completely on ADAL to do the heavy lifting for everything that's related to Token acquisition and lifecycles.

Our Web Applications are generally Multi Tenants/Self Serve with a Microsoft Account and have Background Workers/Daemons components. The In Memory singleton is not suited for scaling out and WebApps restart scenarios.

We've been using basically the same class/pattern for the past 2 years:

• Inherit from TokenCache
  
  
  
  • Scope each instance of this TokenCache to a single user
  
  
• Hook into this.BeforeAccess
  
  
  
  • Fetch the Serialized Cache bits from Redis/Blob/KeyVault
  
  
  • call this.Deserialize to initialize or set the TokenCache
  
  
• Hook into this.AfterAccess
  
  
  
  • call this.Serialize to get the bits of the TokenCache
  
  
  • Push the Serialized Cache bits to Redis/Blob/KeyVault
  
  

This method/pattern is also highlighted here: https://dzimchuk.net/adal-distributed-token-cache-in-asp-net-core/

the NotificationArgs class could contain the deserialized value of the current cache (in the case of OnAfterAccess) and a place to set the serialized value of the cache (in the case of OnBeforeAccess)

Personally, I think it wouldn't be very intuitive to set the state of the cache in the NotificationArgs object. It'd be much more convenient to have the same function but with an Async signature. I also wonder how it'd behave if you'd both call this.Serialize and set the cache in NotificationArgs.

[MarkZuber]

Thanks @AlexandreArpin for your detailed info.

The problem with calling Serialize() within the async callback is re-entrant locking. At serialize time we have to lock. And we're also in a lock when calling you back. But because we're async, I'm looking at using SemaphoreSlim which isn't re-entrant. That's where I was looking at the possibility of doing the serialization and passing that into the args. Agree that setting on return is awkward, perhaps we could have a slightly different callback method for the OnAfterAccess that returns a class with the byte arrays for the various formats in it.

[MarkZuber]

Correction to above. I believe this is possible to just call the (de)serialize methods on the ITokenCache that's passed into the callback even when it's async.

[MarkZuber]

https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/pull/1088

We'd love feedback from the community on design and approach here as well.

[dmcweeney]

Our scenario is quite similar. The web apps are multitenant with MS Work account built on ASP.Net Core.
Using MSAL 2.5 (currently).
Token Cache storage uses Redis Cache vis the IDistributedCache abstraction.
Serialize/Deserialize hooks as per @AlexandreArpin comment.

One thing we also do is use the IDataProtectionProvider for protect/unprotect of the values that are serialized. Not sure if this would cause issues if moving values in the Args param.

[jennyf19]

Fixed in MSAL 4.0.0 release

[dmcweeney]

Thank you...