StringValues Add IsNull, always set single item to _value#323
Conversation
|
@benaadams did you have to reformat all of the methods 🤣 |
Better? 😄 |
|
Then can use |
|
Any perf results? |
This PR is from your point in aspnet/KestrelHttpServer#2347 (comment) So the perf would be from a follow on change in Kestrel changing header existence checks from the equivalent of if ((_value != null ? 1 : (_values?.Length ?? 0)) > 0)to if (!(_value == null && _values == null)) |
| { | ||
| _value = null; | ||
| _values = values; | ||
| if (values == null || values.Length == 0) |
There was a problem hiding this comment.
Maybe add a comment explaining that if _values isn't set to null when values.Length is zero, the IsNull property will be broken.
|
CI was slow... |
|
Added change to remove nulls from arrays to cover the corner case of nulls getting in via This also means the |
| } | ||
| else | ||
| { | ||
| // Remove any nulls from the array else counts will be off |
There was a problem hiding this comment.
This is beyond StringValues' purview to modify the input like this. And as far as we know this is a rare corner case so why put the extra effort into it? It's not even worth writing the tests for. None of your other changes in this PR depend on this filter, correct?
There was a problem hiding this comment.
Its an edge case with consequences.
The string[] ctor is the only way null can appear with a count
new StringValues(new []{(string)null, (string)null, (string)null })Will report .Count == 3
However, it will also output zero headers; so is it really 3 headers?
As null can get into StringValues values every header value has to be null checked in Kestrel to see if it is null or not before outputting it.
This commit 6a5e5f1 means the previous will report .Count == 0 and all the null checks can be removed from Kestrel for header output.
There was a problem hiding this comment.
Common case is there aren't nulls and the string[] ctor is not used; so its pushing to cost for potential null values coming from that .ctor into the .ctor; rather than having guards spread everywhere.
There was a problem hiding this comment.
The constructor is called from an implicit converter, it shouldn't be doing non-trivial transformations. StringValues also only wraps the array so you can't prevent those values from becoming null after the constructor.
Since Kestrel needs the values to not be null then it should do a one time filter, possibly via a helper method provided by StringValues like WithoutNulls or GetNonNulls.
There was a problem hiding this comment.
Since Kestrel needs the values to not be null then it should do a one time filter, possibly via a helper method provided by StringValues like WithoutNulls or GetNonNulls.
That wouldn't achieve any real benefit over what exists now (and likely would be more expensive)
Reverted
This reverts commit 6a5e5f1.
|
Still LGTM |
| } | ||
|
|
||
| if (obj is string) | ||
| if (obj is string st) |
|
Whoops. I didn't squash this. Sorry about the git history. |
|
FYI: This change regressed StringValues.Empty. It used to be equivalent to new string[0], but now it's null. It causes the same problem for MVC's ValueProviderResult.None. @rynowak |
|
@benaadams heads up. We're going to revert this and reintroduce it more cautiously by doing full universe builds before merging. |
Makes sense, a broken build is no place to be... |
|
Reverted. 43ad60b |
ghost
locked as resolved and limited conversation to collaborators
4 participants
No description provided.