[Discussion] HeaderNames now contains static readonly fields instead of const fields

[halter73]

The static Microsoft.Net.Http.Headers.HeaderNames class contains string fields representing various common header names (e.g. HeaderNames.Origin). Starting in ASP.NET Core 3.0 Preview 5, these will change from const fields to static readonly fields.

While this is not a binary breaking change, source code that used these fields as an argument attribute, a case in a switch statement, or when defining another constant will no longer be able to do so. To work around this break, developers can switch to using self-defined header name constants or string literals.

---

This is the discussion issue for aspnet/Announcements#356.

[NickCraver]

Hmmm, this will break quite a few case statements across quite a few apps (choosing which headers we respect/proxy and which we don't). I'm curious: what was the reasoning here?

[jongalloway]

I think it's due to this change, correct @benaadams?

#9341

[halter73]

Yep. This was change was made as part of #9341 to improve performance.

I hadn't considered the source break for switch/case statements. I'll add that to the announcement.

[NickCraver]

Aye that's it - just pinged Ben for the same :)

I get the perf win, but it doesn't seem like switch/case was even mentioned in the breaking change considerations and that surprises me a bit. Any time we're dealing with n things and handling some of them, that's definitely a use case. I'd be happy if these are defined elsewhere, but this just makes everyone using them in any const way duplicate in their code instead of once in the framework.

It seems like we've progressed and regressed several times with headers and status codes. It's disappointing to see the regressions continue there.

[benaadams]

Presumably the case statements aren't doing IgnoreCase comparisons?

As per RFC 7230 3.2. Header Fields are case-insensitive; so if you were switching on header "Accept-Encoding" it wouldn't match "accept-encoding"?

Previously technically you should be doing

if (string.Equals(HeaderNames.AcceptEncoding, headerName, StringComparison.OrdinalIgnoreCase))

Now you can use the faster

if (ReferenceEquals(HeaderNames.AcceptEncoding, headerName) ||
    string.Equals(HeaderNames.AcceptEncoding, headerName, StringComparison.OrdinalIgnoreCase))

And skip OrdinalIgnoreCase in the general case

[halter73]

Interestingly enough, depending on where you're getting the header name value is coming from, it will improve the performance of your code to switch from a switch statement to if/else statements that use ReferenceEquals.

Edit: You could also fall back to a slower string.Equals OrdinalIgnoreCase if ReferenceEquals fails like @benaadams suggests if you don't know where the header name value originated.

[NickCraver]

That's going to depend on the number of cases in the switch ultimately, I think the current threshold is 7 before it does lookup conversion at compile time. Regardless, it's not about perf, it's the loss of a very simple, very clean switch (even more so with expression bodies).

Instead of this:

switch (header)
{
  case HeaderNames.Accept:
  case HeaderNames.AcceptCharset:
  case HeaderNames.AcceptEncoding:
  case HeaderNames.AcceptLanguage:
  case HeaderNames.AcceptRanges:
    return true;
  default:
    return false;
}

...we now have something far more verbose and confusing.

The most reasonable thing we can do is define the constants ourselves in a file, and that's just unfortunate. We've gone back and forth with status codes across various APIs over the years and I was hoping we had stopped doing that.

When the team is designing for performance above most priorities, please don't lose sight that maintainability is more important elsewhere. It'd be very nice if these were still constants available. The statics could reference the constants to ensure they stay in sync.

[NickCraver]

As per RFC 7230 3.2. Header Fields are case-insensitive; so if you were switching on header "Accept-Encoding" it wouldn't match "accept-encoding"?

Yep that's fair, but in our case they're normalized and predictable. I get that we're a rare use case and we'll make constants. My point here was: switch/case needs to be considered on breaks and I don't see that it was. I wanted to raise it so that it's considered next time.

[benaadams]

Regardless, it's not about perf, it's the loss of a very simple, very clean switch (even more so with expression bodies).

Could use a static array?

readonly static string[] s_acceptableHeaders =
{
    HeaderNames.Accept,
    HeaderNames.AcceptLanguage,
    HeaderNames.AcceptCharset,
    HeaderNames.AcceptEncoding
};

public bool IsHeaderAcceptable(string headerName, out string normalizedName)
{
    var acceptableHeaders = s_acceptableHeaders;

    // Check for ReferenceEquals first
    for (var i = 0; i < acceptableHeaders.Length; i++)
    {
        if (ReferenceEquals(headerName, acceptableHeaders[i]))
        {
            normalizedName = acceptableHeaders[i];
            return true;
        }
    }

    // Check for OrdinalIgnoreCase
    for (var i = 0; i < acceptableHeaders.Length; i++)
    {
        if (string.Equals(HeaderNames.AcceptCharset, headerName, StringComparison.OrdinalIgnoreCase))
        {
            normalizedName = acceptableHeaders[i];
            return true;
        }
    }

    normalizedName = headerName;
    return false;
}

My point here was: switch/case needs to be considered on breaks and I don't see that it was. I wanted to raise it so that it's considered next time.

That's fair

[Daniel15]

Does this mean that constants for strings used across multiple assemblies are a bad idea in general, and we should use static readonly fields?

Does .NET not use string interning for constant string literals? This seems like an optimization the compiler should be able to do (even if not by default, perhaps using an attribute on the string)

[benaadams]

Does .NET not use string interning for constant string literals? This seems like an optimization the compiler should be able to do

It does and all the strings in a single assembly will be identical.

Does this mean that constants for strings used across multiple assemblies are a bad idea in general ...

Cross assembly public const string get baked into the callsite of the using assembly (but remain deduped in that assembly). To intern cross-assembly the Jit would have to dedupe on assembly load; which would slow down startups; and the C# compiler marks the assemblies with CompilationRelaxations.NoStringInterning https://github.com/dotnet/coreclr/issues/23969#issuecomment-483023276 which means its up to the runtime what it wants to do; and as I understand it the Core runtime prefers faster startup.

Equality tests are quite fast; and while this has an impact its still happily doing 16,855,482 OrdinalIgnoreCase string compares per second on a single thread before this change; so it might not be something to worry about, but YMMV

[Genbox]

Is there a particular reason why the header names are not enum values?

I suspect it is because the header values themselves are used for parsing/creating HTTP headers, but in that case, I think it should be an implementation detail that they are strings.

As for using an enum instead, you have the benefit of compatibility, speed (because of int comparisons and usability. Usability especially, since you can switch/case the values easily and APIs can take in an enum value instead of string.