Do not reorder HTTP header values

[feiyun0112]

fix #63833

[ghost]

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

---

fix #63833 & #63831

Author:	feiyun0112
Assignees:	-
Labels:	area-System.Net.Http
Milestone:	-

[MihaZupan]

This will now additionally allocate a List<HeaderStoreItemInfoValue>, HeaderStoreItemInfoValue[] and HeaderStoreItemInfoValue even if only a single valid value was added.

I think we should keep the field as object and allocate the List<object> on demand if more than 1 value is added.

We can also avoid allocating the HeaderStoreItemInfoValue for each parsed value. Instead, we can store the values as objects like before, but use a new internal record InvalidValue(string Value) to differentiate between parsed/invalid values. I.e. only paying the extra cost for invalid values.

I also don't believe this will solve #63831 as that deals with the enumeration order when values are both parsed and unparsed.

[MihaZupan]

What I meant is that you would have a field

internal object ParsedAndInvalidValues;

where that object can be null/object/InvalidValue/List<object>.

If only a single value is added (common case), the field will store an InvalidValue or a different object (e.g. a boxed DateTime value).
The List<object> is only allocated if more values are added.

public bool ContainsParsedValues()
{
    object parsedAndInvalidValues = ParsedAndInvalidValues;
    if (parsedAndInvalidValues is null)
    {
        return false;
    }

    if (parsedAndInvalidValues is List<object> list)
    {
        foreach (object value in list)
        {
            if (value is not InvalidValue)
            {
                return true;
            }
        }

        return false;
    }
    else
    {
        return parsedAndInvalidValues is not InvalidValue;
    }
}

[MihaZupan]

This is looking a lot better! Thanks for sticking with me here :)

The main missing parts here are changes to HttpHeaderParsers that rely on the storeValue passed to them. Before, they may have been relying on there being a single value, but now it may be a list that includes invalid values.
For example the CacheControlHeaderParser has to be updated to account for the possibility of being passed a List when extracting a possible existing CacheControlHeaderValue. Having more test coverage here would also be great.

[MihaZupan]

The parsers should still be updated to account for InvalidValues. See #65249 (review)

[geoffkizer]

I don't think this fixes #63831, does it? At least, I didn't see where, and I didn't see a test case for it. Did I miss it?

[MihaZupan]

I don't think this fixes #63831, does it?

It does not. I removed it from the PR description to avoid confusion.

[MihaZupan]

@feiyun0112 It appears this comment has been marked as resolved multiple times - is there an associated change I missed?

[MihaZupan]

Thanks @feiyun0112

[MihaZupan]

/azp run runtime-libraries-coreclr outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).