Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Async versions of BeforeAccess and AfterAccess #481

Closed
dmcweeney opened this issue Oct 13, 2017 · 16 comments
Closed

Async versions of BeforeAccess and AfterAccess #481

dmcweeney opened this issue Oct 13, 2017 · 16 comments

Comments

@dmcweeney
Copy link

@dmcweeney dmcweeney commented Oct 13, 2017

Hi,

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

Thanks

Donal

@jmprieur

This comment has been minimized.

Copy link
Contributor

@jmprieur jmprieur commented Oct 23, 2017

@dmcweeney can you please explain your scenario?

@dmcweeney

This comment has been minimized.

Copy link
Author

@dmcweeney dmcweeney commented Oct 25, 2017

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

This comment has been minimized.

Copy link
Contributor

@jmprieur jmprieur commented Oct 25, 2017

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

@AlexandreArpin

This comment has been minimized.

Copy link

@AlexandreArpin AlexandreArpin commented Oct 31, 2017

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

This comment has been minimized.

Copy link

@juunas11 juunas11 commented Mar 9, 2018

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

@henrik-me

This comment has been minimized.

Copy link
Collaborator

@henrik-me henrik-me commented Mar 26, 2019

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

This comment has been minimized.

Copy link
Member

@bgavrilMS bgavrilMS commented Apr 8, 2019

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

This comment has been minimized.

Copy link

@juunas11 juunas11 commented Apr 9, 2019

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.

@bgavrilMS bgavrilMS removed the In Progress label Apr 9, 2019
@jennyf19 jennyf19 modified the milestones: 3.0.5, 3.0.6, 3.0.7 Apr 10, 2019
@MarkZuber

This comment has been minimized.

Copy link
Contributor

@MarkZuber MarkZuber commented Apr 16, 2019

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

This comment has been minimized.

Copy link

@AlexandreArpin AlexandreArpin commented Apr 17, 2019

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

This comment has been minimized.

Copy link
Contributor

@MarkZuber MarkZuber commented Apr 17, 2019

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

This comment has been minimized.

Copy link
Contributor

@MarkZuber MarkZuber commented Apr 17, 2019

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

This comment has been minimized.

Copy link
Contributor

@MarkZuber MarkZuber commented Apr 17, 2019

#1088

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

@dmcweeney

This comment has been minimized.

Copy link
Author

@dmcweeney dmcweeney commented Apr 17, 2019

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.

@MarkZuber MarkZuber modified the milestones: 3.0.8, 3.0.9 Apr 29, 2019
@MarkZuber MarkZuber added the Fixed label May 8, 2019
@MarkZuber MarkZuber closed this May 8, 2019
@jmprieur jmprieur modified the milestones: 3.1, 4.0 May 29, 2019
@jennyf19

This comment has been minimized.

Copy link
Collaborator

@jennyf19 jennyf19 commented Jun 4, 2019

Fixed in MSAL 4.0.0 release

@dmcweeney

This comment has been minimized.

Copy link
Author

@dmcweeney dmcweeney commented Jun 4, 2019

Thank you...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.