From: Stephen Toub Date: Tue, 15 Jun 2021 00:15:32 +0000 (-0400) Subject: Remove outdated lock from ParseRawHeaderValues (#54130) X-Git-Tag: submit/tizen/20210909.063632~775 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=bfb8d6bfd3c708e148755caa3e3ae916cdef7ed9;p=platform%2Fupstream%2Fdotnet%2Fruntime.git 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). --- diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs index 4c67e98..fd1074f 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs @@ -37,34 +37,19 @@ namespace System.Net.Http.Headers private readonly HttpHeaderType _allowedHeaderTypes; private readonly HttpHeaderType _treatAsCustomHeaderTypes; - /// Whether to force values in to be wrapped in objects. - /// - /// 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 objects in the , - /// but that means the value must be a rather than a raw string, which we prefer to store for - /// unvalidated additions. To work around that, for the HttpClient.DefaultRequestHeaders collection sets - /// to true, which will cause additions to always be wrapped, even if we otherwise wouldn't need them to be. - /// - 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? HeaderStore => _headerStore; @@ -157,7 +142,7 @@ namespace System.Net.Http.Headers 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 @@ namespace System.Net.Http.Headers // 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 @@ namespace System.Net.Http.Headers 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 @@ namespace System.Net.Http.Headers 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 @@ namespace System.Net.Http.Headers 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? rawValues = info.RawValue as List; + List? rawValues = info.RawValue as List; - 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; } } diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpRequestHeaders.cs b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpRequestHeaders.cs index 20c6636..f3035cc 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpRequestHeaders.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpRequestHeaders.cs @@ -261,11 +261,6 @@ namespace System.Net.Http.Headers { } - internal HttpRequestHeaders(bool forceHeaderStoreItems) - : base(HttpHeaderType.General | HttpHeaderType.Request | HttpHeaderType.Custom, HttpHeaderType.Response, forceHeaderStoreItems) - { - } - internal override void AddHeaders(HttpHeaders sourceHeaders) { base.AddHeaders(sourceHeaders); diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs b/src/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs index cccc411..514744f 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs @@ -43,7 +43,7 @@ namespace System.Net.Http } public HttpRequestHeaders DefaultRequestHeaders => - _defaultRequestHeaders ??= new HttpRequestHeaders(forceHeaderStoreItems: true); + _defaultRequestHeaders ??= new HttpRequestHeaders(); public Version DefaultRequestVersion {