dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.07k stars 4.69k forks source link

ClaimsPrincipal.SelectPrimaryIdentity allocates unnecessary enumerator #107861

Open idg10 opened 2 weeks ago

idg10 commented 2 weeks ago

Description

The ClaimsPrincipal.SelectPrimaryIdentity method (which is used by the public Identity property) always allocates an enumerator, when this should never be necessary.

The method is declared as taking an IEnumerable<ClaimsIdentity>. In normal usage, this argument is always the List<ClaimsIdentity> from the _identities field. (There's an obscure situation in which it might not be: user code might retrieve the static PrimaryIdentitySelector property and invoke it passing in their own IEnumerable<ClaimsIdentity>.)

This means that the foreach inside SelectPrimaryIdentity can't take advantage of the zero-allocation enumeration normally possible with a List<T>, because the CLR has to generate code that can cope with any IEnumerable<T>. So the overwhelming majority of use cases pay an allocation penalty that is unnecessary.

This could be modified to detect an IList as a special case to avoid the allocation.

Configuration

.NET 8.0.8, SDK 8.0.400. All architectures, any OS.

Regression?

No.

Analysis

We saw this allocation in a performance analysis. We were able to prevent it by writing this helper:

public static IIdentity? GetPrimaryIdentity(this ClaimsPrincipal claimsPrincipal)
{
    if (claimsPrincipal.Identities is IList<ClaimsIdentity> identities)
    {
        for (int i = 0; i < identities.Count; i++)
        {
            if (identities[i] is not null)
            {
                return identities[i];
            }
        }
    }

    return claimsPrincipal.Identity;
}

and using it instead of user.Identity. Running our performance analysis again with this in place showed that we no longer saw an enumerator being allocated.

dotnet-policy-service[bot] commented 2 weeks ago

Tagging subscribers to this area: @dotnet/area-system-security, @bartonjs, @vcsjones See info in area-owners.md if you want to be subscribed.