Remove outdated lock from ParseRawHeaderValues (#54130)

This lock existed to protect HttpClient.DefaultRequestHeaders.  While headers collections aren't meant to be thread-safe, out of necessity the design of DefaultRequestHeaders required this lock be in place, with multiple concurrent requests all potentially enumerating DefaultRequestHeaders in order to copy its contents into the outgoing request.  Such enumeration could trigger parsing, which could trigger the same object to be mutated from multiple threads to store the parsed result.  But as of https://github.com/dotnet/runtime/pull/49673, we no longer force parsing of the DefaultRequestHeaders, instead preferring to just transfer everything over as-is.  With that, there shouldn't be any concurrent mutation of these objects (and if there is, it's user error doing their own concurrent enumeration of a header collection).

src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs

@@ -37,34 +37,19 @@ public abstract class HttpHeaders : IEnumerable<KeyValuePair<string, IEnumerable
 
         private readonly HttpHeaderType _allowedHeaderTypes;
         private readonly HttpHeaderType _treatAsCustomHeaderTypes;
-        /// <summary>Whether to force values in <see cref="_headerStore"/> to be wrapped in <see cref="HeaderStoreItemInfo"/> objects.</summary>
-        /// <remarks>
-        /// In general, header and collection types in System.Net.Http are not thread-safe: it's an error to read/write the same header
-        /// collection on multiple threads, and even to enumerate a single header collection from multiple threads concurrently; doing
-        /// so may lazily-initialize various properties and structures.  However, there is one collection exempt from this based purely
-        /// on necessity: HttpClient.DefaultRequestHeaders.  DefaultRequestHeaders is enumerated to add all of its headers into each
-        /// request, and since requests may be sent on the same HttpClient instance concurrently, this collection may be enumerated
-        /// concurrently.  As such, we need to ensure that any mutation performed on DefaultRequestHeaders while enumerating is done in
-        /// a thread-safe way.  This is achieved by locking on the <see cref="HeaderStoreItemInfo"/> objects in the <see cref="_headerStore"/>,
-        /// but that means the value must be a <see cref="HeaderStoreItemInfo"/> rather than a raw string, which we prefer to store for
-        /// unvalidated additions.  To work around that, for the HttpClient.DefaultRequestHeaders collection sets <see cref="_forceHeaderStoreItems"/>
-        /// to true, which will cause additions to always be wrapped, even if we otherwise wouldn't need them to be.
-        /// </remarks>
-        private bool _forceHeaderStoreItems;
 
         protected HttpHeaders()
             : this(HttpHeaderType.All, HttpHeaderType.None)
         {
         }
 
-        internal HttpHeaders(HttpHeaderType allowedHeaderTypes, HttpHeaderType treatAsCustomHeaderTypes, bool forceHeaderStoreItems = false)
+        internal HttpHeaders(HttpHeaderType allowedHeaderTypes, HttpHeaderType treatAsCustomHeaderTypes)
         {
             // Should be no overlap
             Debug.Assert((allowedHeaderTypes & treatAsCustomHeaderTypes) == 0);
 
             _allowedHeaderTypes = allowedHeaderTypes & ~HttpHeaderType.NonTrailing;
             _treatAsCustomHeaderTypes = treatAsCustomHeaderTypes & ~HttpHeaderType.NonTrailing;
-            _forceHeaderStoreItems = forceHeaderStoreItems;
         }
 
         internal Dictionary<HeaderDescriptor, object>? HeaderStore => _headerStore;
@@ -157,7 +142,7 @@ internal bool TryAddWithoutValidation(HeaderDescriptor descriptor, string? value
             else
             {
                 // The header store did not contain the header.  Add the raw string.
-                _headerStore.Add(descriptor, _forceHeaderStoreItems ? new HeaderStoreItemInfo { RawValue = value } : (object)value);
+                _headerStore.Add(descriptor, value);
             }
 
             return true;
@@ -324,10 +309,7 @@ internal string GetHeaderString(HeaderDescriptor descriptor)
                     // To retain consistent semantics, we need to upgrade a raw string to a HeaderStoreItemInfo
                     // during enumeration so that we can parse the raw value in order to a) return
                     // the correct set of parsed values, and b) update the instance for subsequent enumerations
-                    // to reflect that parsing.  It is safe to write back into the dictionary here because
-                    // the only collection that can be enumerated concurrently is HttpClient.DefaultRequestHeaders,
-                    // and all values in it will be HeaderStoreItemInfo.
-                    Debug.Assert(!_forceHeaderStoreItems);
+                    // to reflect that parsing.
                     _headerStore[descriptor] = info = new HeaderStoreItemInfo() { RawValue = value };
                 }
 
@@ -337,9 +319,6 @@ internal string GetHeaderString(HeaderDescriptor descriptor)
                 if (!ParseRawHeaderValues(descriptor, info, removeEmptyHeader: false))
                 {
                     // We have an invalid header value (contains invalid newline chars). Delete it.
-                    // Note that ParseRawHeaderValues locks on the info object, such that only a single
-                    // call to it with the same info will return false, which makes this removal safe to
-                    // do even for HttpClient.DefaultRequestHeaders, which may be enumerated concurrently.
                     _headerStore.Remove(descriptor);
                 }
                 else
@@ -561,7 +540,7 @@ internal virtual void AddHeaders(HttpHeaders sourceHeaders)
                     else
                     {
                         Debug.Assert(sourceValue is string);
-                        _headerStore.Add(header.Key, _forceHeaderStoreItems ? new HeaderStoreItemInfo { RawValue = sourceValue } : sourceValue);
+                        _headerStore.Add(header.Key, sourceValue);
                     }
                 }
             }
@@ -731,43 +710,38 @@ private bool TryGetAndParseHeaderInfo(HeaderDescriptor key, [NotNullWhen(true)]
 
         private bool ParseRawHeaderValues(HeaderDescriptor descriptor, HeaderStoreItemInfo info, bool removeEmptyHeader)
         {
-            // Prevent multiple threads from parsing the raw value at the same time, or else we would get
-            // false duplicates or false nulls.
-            lock (info)
+            // Unlike TryGetHeaderInfo() this method tries to parse all non-validated header values (if any)
+            // before returning to the caller.
+            if (info.RawValue != null)
             {
-                // Unlike TryGetHeaderInfo() this method tries to parse all non-validated header values (if any)
-                // before returning to the caller.
-                if (info.RawValue != null)
-                {
-                    List<string>? rawValues = info.RawValue as List<string>;
+                List<string>? rawValues = info.RawValue as List<string>;
 
-                    if (rawValues == null)
-                    {
-                        ParseSingleRawHeaderValue(descriptor, info);
-                    }
-                    else
-                    {
-                        ParseMultipleRawHeaderValues(descriptor, info, rawValues);
-                    }
+                if (rawValues == null)
+                {
+                    ParseSingleRawHeaderValue(descriptor, info);
+                }
+                else
+                {
+                    ParseMultipleRawHeaderValues(descriptor, info, rawValues);
+                }
 
-                    // At this point all values are either in info.ParsedValue, info.InvalidValue, or were removed since they
-                    // contain invalid newline chars. Reset RawValue.
-                    info.RawValue = null;
+                // At this point all values are either in info.ParsedValue, info.InvalidValue, or were removed since they
+                // contain invalid newline chars. Reset RawValue.
+                info.RawValue = null;
 
-                    // During parsing, we removed the value since it contains invalid newline chars. Return false to indicate that
-                    // this is an empty header. If the caller specified to remove empty headers, we'll remove the header before
-                    // returning.
-                    if ((info.InvalidValue == null) && (info.ParsedValue == null))
+                // During parsing, we removed the value since it contains invalid newline chars. Return false to indicate that
+                // this is an empty header. If the caller specified to remove empty headers, we'll remove the header before
+                // returning.
+                if ((info.InvalidValue == null) && (info.ParsedValue == null))
+                {
+                    if (removeEmptyHeader)
                     {
-                        if (removeEmptyHeader)
-                        {
-                            // After parsing the raw value, no value is left because all values contain invalid newline
-                            // chars.
-                            Debug.Assert(_headerStore != null);
-                            _headerStore.Remove(descriptor);
-                        }
-                        return false;
+                        // After parsing the raw value, no value is left because all values contain invalid newline
+                        // chars.
+                        Debug.Assert(_headerStore != null);
+                        _headerStore.Remove(descriptor);
                     }
+                    return false;
                 }
             }
 

src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpRequestHeaders.cs

@@ -261,11 +261,6 @@ internal HttpRequestHeaders()
         {
         }
 
-        internal HttpRequestHeaders(bool forceHeaderStoreItems)
-            : base(HttpHeaderType.General | HttpHeaderType.Request | HttpHeaderType.Custom, HttpHeaderType.Response, forceHeaderStoreItems)
-        {
-        }
-
         internal override void AddHeaders(HttpHeaders sourceHeaders)
         {
             base.AddHeaders(sourceHeaders);

src/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs

@@ -43,7 +43,7 @@ public static IWebProxy DefaultProxy
         }
 
         public HttpRequestHeaders DefaultRequestHeaders =>
-            _defaultRequestHeaders ??= new HttpRequestHeaders(forceHeaderStoreItems: true);
+            _defaultRequestHeaders ??= new HttpRequestHeaders();
 
         public Version DefaultRequestVersion
         {