[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.

[Genbox]

I made a quick benchmark to show the benefits from a performance perspective.

• StaticStringEqual: A.Equals(static B, OrdinalIgnoreCase)
• ConstStringEqual: A.Equals(const B, OrdinalIgnoreCase)
• ReferenceStringEqual: ReferenceEquals(A, A)
• StaticEnumEqual: enum.A == static enum.A
• ConstEnumEqual: enum.A == const enum.A

Note: As you can see, ConstEnumEqual is compiled to 'return true'.

[NickCraver]

Headers must be text because they are text in the HTTP spec itself. They have to be treated as text in so many ways...and they are not finite. You can create a custom header for anything and any reason.

[Genbox]

@NickCraver while they are not finite, they are quite resistant to change. The constant strings are subject to the same changes as an enum would be.

Edit: I'm not saying the API should work on enums only. People should be free to choose which headers they would like to include, whether they are custom, RFC based or comes from a spec by a company. However, the common case is setting accept, modified-on etc. It would improve speed, usability and be non-breaking going forward for the common usage pattern.

As for using them as strings, I agree that there is a need for a list of those strings somewhere, just not in the public API. I very rarely see someone use the strings provided in HeaderNames because the API takes in strings instead of an enum. Not only does it invite a lot of bugs due to misspellings/refactorings, but the optimization discussed in this announcement is bypassed.

[Bartmax]

this is the same reason why IdentityConstants (hint: the class name has constants on it) doesn't have constants?

I agree with @NickCraver this is a breaking change in the sense that I also use normalized headers and don't need comparison and I do have ifs.

const gives us a very clean structure to work with but I always worried about the false sense of "security" doing switch(es) on headers and not considering casing.

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

I feel this too ☝️

[benaadams]

ConstEnumEqual: enum.A == const enum.A would never apply as the header is given as a variable; so the comparision with this change would be ReferenceStringEqual: ReferenceEquals(A, A) and StaticEnumEqual: enum.A == static enum.A which are mostly the same from your benchmark.

However that is a factor of x 8 compared to ConstStringEqual: A.Equals(const B, OrdinalIgnoreCase) which was before this change.

[Genbox]

@benaadams I included the last benchmark mostly for completeness and I agree with your assessment. This is quite a micro-optimization and only applicable due to the way the JIT compiler inline constants. Having an enum instead makes the comparison constant-time with no string allocation needed.

[poke]

Wouldn’t it be possible to define both constants and readonly fields for this? So people could reference constants if they need to without having to redefine them themselves.

public const string AcceptConstant = "Accept";
public static readonly string Accept = AcceptConstant;

But that would probably degrade discoverability of this in general :/

@Bartmax

this is the same reason why IdentityConstants (hint: the class name has constants on it) doesn't have constants?

I think a reason why many of the internal constants are in fact not constants but static readonly fields is that this makes them easier to recognize when debugging. When you are using a constant, that constant value will be emitted in IL; but for static readonly fields you get an actual reference to the field. And a ApplicationScheme is a bit more self-explaining than a Identity.Application (the value is actually an irrelevant implementation detail)

[mrmartan]

There is the MVC use-case which leads to defining the constants in applications, repeatedly.

public IActionResult GetSometing(
    [FromHeader(Name = HeaderNames.AcceptLanguage)] string acceptLanguage = "")
{
}

This does not compile now.

[CumpsD]

I just upgraded to .NET Core 3.1 today and was faced with 350 errors of An attribute argument must be a constant expression, typeof expression or array creation expression of an attribute parameter type 👎

Most of them cases like [FromHeader(Name = HeaderNames.IfNoneMatch)]

For whoever needs it:

public static partial class HeaderNames
    {
        public const string Accept = "Accept";
        public const string AcceptCharset = "Accept-Charset";
        public const string AcceptEncoding = "Accept-Encoding";
        public const string AcceptLanguage = "Accept-Language";
        public const string AcceptRanges = "Accept-Ranges";
        public const string AccessControlAllowCredentials = "Access-Control-Allow-Credentials";
        public const string AccessControlAllowHeaders = "Access-Control-Allow-Headers";
        public const string AccessControlAllowMethods = "Access-Control-Allow-Methods";
        public const string AccessControlAllowOrigin = "Access-Control-Allow-Origin";
        public const string AccessControlExposeHeaders = "Access-Control-Expose-Headers";
        public const string AccessControlMaxAge = "Access-Control-Max-Age";
        public const string AccessControlRequestHeaders = "Access-Control-Request-Headers";
        public const string AccessControlRequestMethod = "Access-Control-Request-Method";
        public const string Age = "Age";
        public const string Allow = "Allow";
        public const string Authority = ":authority";
        public const string Authorization = "Authorization";
        public const string CacheControl = "Cache-Control";
        public const string Connection = "Connection";
        public const string ContentDisposition = "Content-Disposition";
        public const string ContentEncoding = "Content-Encoding";
        public const string ContentLanguage = "Content-Language";
        public const string ContentLength = "Content-Length";
        public const string ContentLocation = "Content-Location";
        public const string ContentMD5 = "Content-MD5";
        public const string ContentRange = "Content-Range";
        public const string ContentSecurityPolicy = "Content-Security-Policy";
        public const string ContentSecurityPolicyReportOnly = "Content-Security-Policy-Report-Only";
        public const string ContentType = "Content-Type";
        public const string Cookie = "Cookie";
        public const string Date = "Date";
        public const string ETag = "ETag";
        public const string Expect = "Expect";
        public const string Expires = "Expires";
        public const string From = "From";
        public const string Host = "Host";
        public const string IfMatch = "If-Match";
        public const string IfModifiedSince = "If-Modified-Since";
        public const string IfNoneMatch = "If-None-Match";
        public const string IfRange = "If-Range";
        public const string IfUnmodifiedSince = "If-Unmodified-Since";
        public const string LastModified = "Last-Modified";
        public const string Location = "Location";
        public const string MaxForwards = "Max-Forwards";
        public const string Method = ":method";
        public const string Origin = "Origin";
        public const string Path = ":path";
        public const string Pragma = "Pragma";
        public const string ProxyAuthenticate = "Proxy-Authenticate";
        public const string ProxyAuthorization = "Proxy-Authorization";
        public const string Range = "Range";
        public const string Referer = "Referer";
        public const string RetryAfter = "Retry-After";
        public const string Scheme = ":scheme";
        public const string Server = "Server";
        public const string SetCookie = "Set-Cookie";
        public const string Status = ":status";
        public const string StrictTransportSecurity = "Strict-Transport-Security";
        public const string TE = "TE";
        public const string Trailer = "Trailer";
        public const string TransferEncoding = "Transfer-Encoding";
        public const string Upgrade = "Upgrade";
        public const string UserAgent = "User-Agent";
        public const string Vary = "Vary";
        public const string Via = "Via";
        public const string Warning = "Warning";
        public const string WebSocketSubProtocols = "Sec-WebSocket-Protocol";
        public const string WWWAuthenticate = "WWW-Authenticate";
    }

[abatishchev]

Please roll it back! The major scenario to use this class in a controller like this:

[HttpGet("{orderId}")]
public async Task<ActionResult<Order>> Get(
    [FromRoute] string orderId,
    [FromHeader(Name = HeaderNames.IfNoneMatch)] string ifNoneMatchETag = null,
    CancellationToken cancellationToken = default)
{
}

And it's impossible.

[Bartmax]

this sounds like a reasonable use case regression, specially since FromHeader can compare ignoring casing which was the only valid point (at least for me) for not using constants.

[gimlichael]

Please roll it back! The major scenario to use this class in a controller like this:

[HttpGet("{orderId}")]
public async Task<ActionResult<Order>> Get(
    [FromRoute] string orderId,
    [FromHeader(Name = HeaderNames.IfNoneMatch)] string ifNoneMatchETag = null,
    CancellationToken cancellationToken = default)
{
}

And it's impossible.

I agree completely; they have not made an improvement - they have made the code less usable and forcing developers to introduce magic strings all in the name of some theoretical performance improvements from static readonly string compared to const. This is a do-over, Microsoft.

[abatishchev]

The general public needs these strings to be constant. So please make them constant again, what they were for so many years. If overall code becomes worse then it's not a performance optimization, it's a regression.

[gimlichael]

All version of the Framework Design Guidelines (incl. 3rd edition) clearly states:

"DO use constant fields for constants that will never change."
"CONSIDER using public static readonly fields for predefined object instances."

Also, several of the authors clearly states that ease of use is far more important than performance, as performance always gets better over time; ease of use doesnt).

So please - align your code changes with the architects behind Framework Design Guidelines before enforcing drastic changes like this.

Thanks.

[markusschaber]

Suggestion: You can still define the static readonly fields which then refer to the constants (either with a schematically derived name, or in a different class / namespace).

[ghost]

Thank you for contacting us. Due to a lack of activity on this discussion issue we're closing it in an effort to keep our backlog clean. If you believe there is a concern related to the ASP.NET Core framework, which hasn't been addressed yet, please file a new issue.

This issue will be locked after 30 more days of inactivity. If you still wish to discuss this subject after then, please create a new issue!

[abatishchev]

Since this is a discussion intended for the general public input it doesn't necessary has to have activity within 30 days, Please reopen.