HttpHeaders should not remove invalid values during validating access

[MihaZupan]

Spinning off the discussion about this specific behavior from #61798.

HttpHeaders will remove the header entry during a validating read if it contained only values with new lines, or had a known parser and was just whitespace.

HttpHeaders headers = new HttpRequestMessage().Headers;

headers.TryAddWithoutValidation("Accept", "invalid with new lines\n"); // or string.Empty
Assert.Equal(1, headers.NonValidated.Count);

Assert.False(headers.TryGetValues("Accept", out _));

// The entry doesn't exist anymore even for NonValidated accesses
Assert.Equal(0, headers.NonValidated.Count);

This means that validated enumeration results in even more side effects to the request/response.
NonValidated enumeration is not able to report all entries if a validated enumeration occurred before it.

I recommend we change the behavior here such that values containing new lines are never implicitly removed. This should result in more consistent behavior and improve the usability of NonValidated.
This would mean that the user may now observe new-line characters & empty values from TryGetValues (given they were added without validation).

As a nice bonus, the fact we remove these entries is the biggest complication in reintroducing support for thread-safe concurrent reads (#66989 (comment)).

[ghost]

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

Issue Details

---

Spinning off the discussion about this specific behavior from #61798.

HttpHeaders will remove the header entry during a validating read if it contained only invalid values.

HttpHeaders headers = new HttpRequestMessage().Headers;

headers.TryAddWithoutValidation("Accept", "invalid");
Assert.Equal(1, headers.NonValidated.Count);

Assert.False(headers.TryGetValues("Accept", out _));

// The entry doesn't exist anymore even for NonValidated accesses
Assert.Equal(0, headers.NonValidated.Count);

This means that validated enumeration results in even more side effects to the request/response.
NonValidated enumeration is not able to report all entries if a validated enumeration occurred before it.

The user may expect that only headers visible during enumeration will be present in the request, which is only partially the case today.
If you don't see a given header name during validating enumeration, you won't see it in the request either. On the other hand, invalid values will still be preserved, as long as there is at least one valid value present.

I recommend we change the behavior here such that invalid values are never implicitly removed. This should result in more consistent behavior and improve the usability of NonValidated.
As a nice bonus, the fact we remove these entries is the biggest complication in reintroducing support for thread-safe concurrent reads.

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

[MihaZupan]

Updated the description to properly note that this only applies to header values that are empty or contain new line characters and were added via TryAddWithoutValidation already.

[karelz]

Triage:

• It is blocker for thread-safety we need (e.g. ConnectCallback's context.InitialRequestMessage has misleading lifetime (not tied to the Connection) #66989)
• Technical breaking change, but closer to common expected behavior

[heathbm]

This appears to be a quick fix, I hope I can help with this

[heathbm]

Should the "or had a known parser and was just whitespace" work be a separate PR?

[MihaZupan]

Should the "or had a known parser and was just whitespace" work be a separate PR?

I think it's closely related and can be the same PR - any implicit Remove calls should be removed.