Re-evaluate sync vs async extension points in AuthenticationHandler

[Tratcher]

AuthenticatoinHandler has several abstract/virtial APIs where it has both a sync and async overload, and the developer can choose to implement one or the other. It's very common for the sync implementation to call the async method and block. The base handler calls the sync and async overloads in different circumstances, such as sync calls from OnSendingHeaders.

Theory 1: Get rid of the sync calls, make OnSendingHeaders async.
Theory 2: Get rid of any async calls that are only called from OnSendingHeaders. Make sure there is only one overload of each extension point, be it sync or async.

[kevinchalet]

The first option is probably the best one! 👍

You should also make ApplyResponseChallenge(Async) and ApplyResponseGrant(Async) non-abstract, just like they were in Katana 3: http://katanaproject.codeplex.com/SourceControl/latest#src/Microsoft.Owin.Security/Infrastructure/AuthenticationHandler.cs

[Tratcher]

Here's a breakdown of the various code paths:

Request:
(Passive) App -> HttpContext.Authenticate/Async -> IAuthenticationHandler.Authenticate/Async -> AuthenticationHandler.Authenticate/Async -> AuthenticateCore/Async
(Active) Middleware.Invoke -> AuthenticationHandler.BaseInitializeAsync -> AuthenticateAsync -> AuthenticateCoreAsync
Response:
OnSendingHeaders -> ApplyResponse -> ApplyResponseCore -> ApplyResponseGrant & ApplyResponseChallenge
TeardownAsync -> ApplyResponseAsync -> ApplyResponseCoreAsync -> ApplyResponseGrantAsync & ApplyResponseChallengeAsync

All sync and async code paths are currently in use internally or exposed to application code so there are no overloads that we can just eliminate.

The problem can be broken down into two parts; the request (Authenticate) and the response (Grant/Challenge).

For the request code path the infrastructure has no dependencies on the sync APIs but they are exposed to application code. We'd have to evaluate the app and Identity usage scenarios to see if we could eliminate the sync code path.

For the response the only sync dependency is OnSendingHeaders. Making OSH async would eliminate the need for the sync overloads. OSH could be fully async except when triggered by sync response stream APIs (Write/Flush), and even then the server would have some discretion (e.g. Helios does write-behind buffering, so it could fire OSH async even on sync code paths).

[kevinchalet]

For the request code path the infrastructure has no dependencies on the sync APIs but they are exposed to application code. We'd have to evaluate the app and Identity usage scenarios to see if we could eliminate the sync code path.

In Katana 3, AuthenticateAsync/AuthenticateCoreAsync had no sync equivalent. Do you remember someone complaining about this lack?

Concerning Identity, they are only using the async version:
https://github.com/aspnet/Identity/blob/dev/src/Microsoft.AspNet.Identity/SignInManager.cs#L218
https://github.com/aspnet/Identity/blob/dev/src/Microsoft.AspNet.Identity/SignInManager.cs#L320
https://github.com/aspnet/Identity/blob/dev/src/Microsoft.AspNet.Identity/SignInManager.cs#L385

[HaoK]

Yup, identity is fully async, I'm not a huge fan of having sync and async overloads either, +1 for eliminating sync if possible

[HaoK]

@Tratcher is this something you think we can/should do? It definitely would make things a lot cleaner

[HaoK]

Note: We will have to convince @lodejard but I think we should try if you agree that this is the 'right' thing to do...

[Tratcher]

@HaoK I tried working through it once without much success. It's hard to get around the OnSendingHeaders code path. Take a look and let me know if you agree/disagree.

[HaoK]

Sounds like this is DOA since we can't get any traction...closing

[kevinchalet]

Any idea why this suggestion was not easy to implement?

[Tratcher]

OnSendingHeaders was the big blocker. It needs to be sync (due to Stream.Write), but we wanted everything else to be async, so we're left with dual code paths.

[kevinchalet]

And calling Wait()/GetAwaiter().GetResult() in Stream.Write was not an option?
Of course, it's not perfect but it has one merit: only developers using the sync write method would pay the price of the blocking OSH callbacks.

[TheBreadGuy]

bit of a noob, here. but, I'm messing with auth middleware. on googling i came to here so thought id post.

On my handler, it is asking for AuthenticateCore as it is abstract, I already have async method, so slightly unsure what to do with it right now. Right, now, some other things are not compiling, so i cant test but once I will let you know happens.

I expect I am doing something completely wrong, and I expect this is totally unrelated to the onheader thing, but thought just say, that asyc/sync. thing can get little confusing.
nl

[Tratcher]

AuthenticateCore is required, AuthenticateCoreAsync is an optional overload if you need to do anything async. See https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNet.Authentication/AuthenticationHandler.cs#L242

[kevinchalet]

@Tratcher no remark concerning my last question? 😄

[Tratcher]

We don't want to have Wait's hidden in the server code, we want them to be a conscious choice at the middleware layer.

[kevinchalet]

Like that? 😄

https://github.com/aspnet/KestrelHttpServer/blob/dev/src/Microsoft.AspNet.Server.Kestrel/KestrelEngine.cs#L102
https://github.com/aspnet/KestrelHttpServer/blob/dev/src/Microsoft.AspNet.Server.Kestrel/Http/FrameResponseStream.cs#L63
https://github.com/aspnet/KestrelHttpServer/blob/dev/src/Microsoft.AspNet.Server.Kestrel/Http/Listener.cs#L112

[Tratcher]

Citing kestrel code isn't going to help your argument, we already know it's broken :-).

[kevinchalet]

Well, I don't have access to aspnet/WebListener or aspnet/Helios, so... 😄

The best argument I have is that currently, everybody is paying the price of a Wait() call when having an authentication middleware in an ASP.NET 5 app, even the ones who use WriteAsync. With this change, only the ones using the synchronous Write would pay for that. And ideally, everybody should be encouraged to use WriteAsync.