UserManager depends on IHttpContextAccessor and It behaves incorrectly when lifetimes don't match up

[tugberkugurlu]

TL;DR; is that UserManager depends on a per-request instance dependency, IHttpContextAccessor, and it can construct itself successfully even if it's (the UserManager) is registered as singleton.

What Happened?

I was seeing System.OperationCanceledException while working with ASP.NET Core Identity (see tugberkugurlu/AspNetCore.Identity.MongoDB#15 for more context). After digging into the issue for a while, I discovered this line: https://github.com/aspnet/Identity/blob/rel/1.1.0-preview1/src/Microsoft.AspNetCore.Identity/UserManager.cs#L46 which made sense and I started belamng my reverse proxy causing requests to be aborted in the middle of the operations. However, it didn't take long to understand it was not the case at all.

The actual problem was related to the fact that I was registering the UserManageras singleton (see https://github.com/tugberkugurlu/AspNetCoreSamples/blob/a4004c851dfbf84fa92891b0d8ef54fa822c6ff6/haproxy-redis-auth/src/Startup.cs#L113) and whereas it was relying on a per-request dependency in a very wrong way 😞 (see https://github.com/aspnet/Identity/blob/rel/1.1.0-preview1/src/Microsoft.AspNetCore.Identity/UserManager.cs#L98)

So, as expected, the CancellationToken property of UserManager entered into the canceled state right after the first request finished, which caused all subsequent calls to the store implementation to receive an already cancelled cancellation token.

What Should Happen Instead

It should have blown up at the startup complaining about the fact that a single instance registration is relying on an instance which is registered with a shorter-lived scoped (per-request).

I am not sure whether it's OK to classify this is a bug but to me, it seems like it really needs fixing ASAP because it's so annoying to debug and understand when you hit this 😞

[pmhsfelix]

• The IHttpContextAccessor implementation should have singleton scope since it uses AsyncLocal to obtain the current context - see [Announcement] The IHttpContextAccessor service is not registered by default Hosting#793
• So, using UserManager also with singleton scope is compatible with the IHttpContextAccessor scope.
• However, UserManager seems to capture the context as a readonly field (https://github.com/aspnet/Identity/blob/rel/1.1.0-preview1/src/Microsoft.AspNetCore.Identity/UserManager.cs#L98), meaning that it should have per request scope.

[tugberkugurlu]

The IHttpContextAccessor implementation should have singleton scope

Thanks, I can remember that thread now. That makes sense.

meaning that it should have per request scope.

At least for now but I believe the way that UserManager acquires IHttpContextAccessor impl is wrong. It should have been done through the ctor as a param and CancellationToken should have been taken on demand from the fresh context each time it's requested.

More importantly, the other annoying thing is the fact that UserManager cares about request abort. It should just receive a CancellationToken through each methods as a parameter and rely on that. So, the consumer should be responsible for retrieving the RequestAbort cancellation token and pass it down (maybe combine that token with your own cancellation token). However, with the current implementation, I feel that integrating UserManager with IHttpContextAccessor couples the cancellation approach too much.

[HaoK]

So there has always been an implicit assumption that the mangers are scoped. @divega do you see any problem with this assumption? We haven't really tried to enable things like singleton lifetimes..

[divega]

Yes, we assume the managers are scoped and I don't think we ever intended them to be used with a different lifetime. Just a few thoughts:

1. We are limited on what we can express regarding the intent for specific types to be registered as services only with specific lifetimes and with validating that services are instantiated only with the right lifetime (e.g. it is very likely an error for a singleton to depends on any scoped service). This seems to be good feedback for the DI repo.
2. Some time ago we intentionally removed CancelationToken arguments from methods on the managers and started relying only on IHttpContextAccessor for cancellation. A bit more information from @tugberkugurlu on why that hurts (besides the coupling feeling too strong) could be really helpful.
3. Unfortunately I can't recall why we made IHttpContextAccessor optional and moreover why we only did it for UserManager in 167bb54 (it is still required for RoleManager and SignInManager). @HaoK do you remember?

[HaoK]

#655 drove why we made it optional

[HaoK]

Its required for SIgnInManager for sure, since that can't work without an HttpContext. We could(should?) have made it optional for RoleManager as well.

[divega]

@HaoK thanks for refreshing my memory. Let's chat about this in person in triage. From reading #655 and linked issues it seems to me that if it is was valid to use UserManager (and probably RoleManager) without an IHttpContextAccessor then we could have considered having CancellationToken parameters back.

[HaoK]

Sure but in that issue it seems more that we don't really care too much if the functionality is lost, so there's certainly no good reason to add a billion overloads to pass around cancellation tokens on the manager APIs.

[HaoK]

We could do something cheap like a new setter on the managers to set your own CancellationToken which would get used instead of the HttpContext's as a cheap way to enable this fringe scenario... but I don't think many (any?) people would use this feature if we added it...

[tugberkugurlu]

Some time ago we intentionally removed CancelationToken arguments from methods on the managers and started relying only on IHttpContextAccessor for cancellation. A bit more information from @tugberkugurlu on why that hurts (besides the coupling feeling too strong) could be really helpful.

@divega It's not much of an issue from the consumption point of view when you know UserManager uses it. However, it's not intuitive at the first. I was surprised to see UM was caring about request abort.

So, it's not a strong case to request it to be removed. However, I would love to hear what made you guys to remove CancelationToken arguments from methods on the managers.