Remove cancellation token hookup for RequestAborted

[HaoK]

Per @davidfowl request, we should remove the logic in the managers that hook up the cancellation tokens for 2.2

https://github.com/aspnet/Identity/blob/dev/src/Identity/AspNetUserManager.cs#L45

any objects @ajcvickers @blowdart ?

[davidfowl]

One of the major driving forces behind this request is the fact that identity cannot be used for tracking users today in a SignalR application because of this logic. When developers try to query a user using the user managed in OnDisconnectedAsync, it blows up because the connection is already disconnected which means the token has already tripped.

[blowdart]

Ah so this is just limited to Identity? What about users not using SignalR and depending on the current behaviour? What sort of existing use do you want to kill just to support SignalR? Why can't SignalR do it another way? What does ASP.NET expose cancellation tokens for?

[davidfowl]

Ah so this is just limited to Identity?

Yes. Identity is the only thing in our stack that implicitly uses the IHttoContextAccessor.

What about users not using SignalR and depending on the current behaviour?

It's a breaking change but my guess is that it won't affect a majority of people using this API. They'll see less exceptions.

What sort of existing use do you want to kill just to support SignalR?

The one risk would be that if the client disconnects, it will no longer cancel the user manager operation. So say your FindUserAsync operation took 5 minutes, it would no longer be automatically cancelled if the client closed the browser (or the reverse proxy dropped the connection). But to be honest, nothing else does this.

Why can't SignalR do it another way?

Because it has nothing to do with SignalR. If you're in an ASP.NET Core application with Mvc, SignalR and Nancy, you get the AspNetUserManager which has this behavior. We could tell users to stop using it and deprecate that type and instead just use the base UserManager.

What does ASP.NET expose cancellation tokens for?

To detect client disconnects. Most APIs take them in as arguments, identity happens to be the only one that uses the implicit token in all database operations. It's most commonly used today when you have a long running operation and you want to cancel it in response to the client going away. We use it today here https://github.com/aspnet/StaticFiles/blob/6b18dea711c510023d458ad8a1deb529e770cb50/src/Microsoft.AspNetCore.StaticFiles/StaticFileContext.cs#L336 (noticed how it's passed in explicitly)

[blowdart]

Well, you know we don't do breaking changes in minor dot releases

cc @DamianEdwards

[davidfowl]

Well, you know we don't do breaking changes in minor dot releases

Yes we do. We do all the time. It's case by case. We broke a bunch of behaviors going from 2.0 to 2.1.

Regardless, I think the risk is extremely low here. I'm willing to have somebody challenge that though

[HaoK]

We could just leave the class alone, and swap out the default behavior to register UserManager, and then the workaround to get it back would be pretty simple, they could just add the old classes back to DI...

[davidfowl]

Yea, so that was my other thought, maybe we just don't register this impl by default anymore. I'm not sure what's more breaking though.

[HaoK]

I agree with you that certainly almost noone is relying on this behavior today, the few issues we've gotten were similar to this one where they wanted to turn off the behavior

[blowdart]

Swapping behaviour would be safer, letting people reenable it if they need it.

[davidfowl]

Yea that’s fine. If you do change the injected type, anyone that was expecting to take an AspNetUserManager would be broken. I dunno if people do that but it’s worth calling out.

[HaoK]

Yeah pretty unlikely since we added that fairly recently in 2.0, and it literally has the same shape as UserManager