Initialize Headers Dictionary Only Once#4853
Uh oh!
There was an error while loading. Please reload this page.
Conversation
markekraus requested review from JamesWTruher, PaulHigin and dantraMSFT as code owners
msftclas commented Sep 16, 2017
@markekraus, |
markekrausforce-pushed the HeadersDictionary branch from dd128ba to ec28224Compare
| } | ||
| } | ||
| privateDictionary<string,IEnumerable<string>>headers=null; |
This code has potential reuse in InvokeRestMethodCommand. Moving it to WebResponseHelper
markekraus commented Sep 17, 2017
I realized in planning for #4845 the headers dictionary logic would need to be available outside of |
iSazonov commented Sep 18, 2017
@markekraus Have we already tests? If so please add the comment in PR description. And about WebResponseHelper too. |
| { | ||
| foreach(varentryinresponse.Content.Headers) | ||
| { | ||
| headers[entry.Key]=entry.Value; |
There was a problem hiding this comment.
Interesting, can there be duplicate headers in response.Headers and response.Content.Headers ? Have second ones a high ptiority?
There was a problem hiding this comment.
No. The headers in Response.Content.Headers will never exist in response.Headers. #4494 added this to address the lack of Content-Type in the WebResponseObject.Headers. I should also say that HttpResponseMessage has it's own conflict resolution for the headers, so we shouldn't have to do anything here.
There was a problem hiding this comment.
Please clarify - my understanding from #4494 is that headers already contains response.Content.Headers, correct?
There was a problem hiding this comment.
That was added in #4494. Previous to that, WebResponseObject.Headers did not contain any of the headers in HttpResponseMessage.Content.Headers. CoreFX has split the response headers so that those related to content are always in HttpResponseMessage.Content.Headers and the rest of the response headers are in HttpResponseMessage.Headers.
To clarify, I'm not adding or changing any of the logic in this PR for the creation of the Dictionary. Just moving it so that a new dictionary is not created on every Get to Headers and to make the logic available outside of WebResponseObject. The existing logic is there on purpose and we have existing tests to ensure it.
There was a problem hiding this comment.
Thanks for clarify.
So foreach (var entry in response.Content.Headers) is duplicate code and could we remove it?
Sorry for late question - if CoreFX split the headers why we join them again? Only for backward compatibility?
There was a problem hiding this comment.
We are joining them for backwards compat. In 5.1 and earlier all headers are in WebResponseObject.Headers. In 6.0 the content headers are buried in WebResponseObject.BaseResponse.Content.Headers unless we promote them to the WebResponseObject.Headers dictionary. One would reasonably expect the Headers property to contain all headers.
There was a problem hiding this comment.
@markekraus can you please add a comment to the code to clarify that there won't be duplicate headers in response.Headers and response.Content.Headers? It would be very helpful to other people who look at the code later.
There was a problem hiding this comment.
@daxian-dbw should I also include a note about the content headers being added to the headers dictionary for backwards compatibility? or is the note about the distinct headers sufficient?
There was a problem hiding this comment.
I think it would also be helpful to mention the backward compatibility issue. Thanks @markekraus!
markekraus commented Sep 18, 2017
@iSazonov I have updated the PR description RE: Testing and move to |
iSazonov commented Sep 21, 2017
@markekraus Thanks for the fix! |
4 participants
Switch the
WebResponseObject.HeadersDictionary to initialize once instead of creating a new dictionary on every get. Moved logic toWebResponseHelper.GetHeadersDictionary()as this code will be reused outside ofWebResponseObject.Closes#3842
This changes internal functionality only. Existing tests for the
Headersproperty are sufficient.