State changes

[brentschmaltz]

Additional Tests
Check for null RedirectToIdentityProviderNotification != null before preparing the call
Additional state can be easily set by user in RedirectToIdentityProviderNotification

[Tratcher]

This if block can be merged with the prior one.

Replace IsNullOrWhiteSpace with IsNullOrEmpty.

[brentschmaltz]

Sure enough, that was a copy paste issue.

[brentschmaltz]

@Tratcher @PinpointTownes to move things forward, I put back the default RedirectToIdentityProvider = notification => Task.FromResult(0);
I still check for null, since the user can (with existing code) set the handler to null.

We can discuss #307 in a normal (whatever that is) timeline.

[kevinchalet]

If you really want to prevent a user from setting notifications to null, why not simply adding a [param: NotNull] attribute on their setter?

[brentschmaltz]

Well, I don't actually. For a very busy server, handling a lot of request. Setting up for notifications that just no-op seems a waste. So I would like to start with them null and take action if the user actually hooked one.

[kevinchalet]

Awaiting a completed task (like the one returned by Task.FromResult) is not that expensive and never causes a thread switching. We're really talking about micro-optimization here (btw, in a code path that shouldn't be hit thousands times a second 😄)

Do you have a benchmark confirming your feeling?

[brentschmaltz]

It is not just the Task.FromResult, but the build up to make the call. We have to new up objects etc.
We took changes like using: string.IsNullOrEmpty rather than string.IsNullOrWhiteSpace just because it seemed like the right thing.
This feels the same way, and is the way traditional asp.net / WIF works.
Look at the number of notifications in AuthenticateAsync. I proposed another one: AuthenticationComplete.

#327

[Eilon]

Checks for whitespace are functionally incorrect in 99% of cases. That is different from doing an optimization where the functionality is identical.

[brentschmaltz]

I am not sure exactly sure what you mean by functionally incorrect or how you came to 99%, but I removed the default task handler and put in a check for null since the existing API allows for the users to set the notification to null.

Is there anything else blocking this PR?

[Eilon]

@brentschmaltz we'll finish this up next week when @Tratcher is back from vacation.

[Tratcher]

Rebased, squashed, and merged.