Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add HeaderEncodingSelector to MultipartContent #39169

Merged
merged 6 commits into from Aug 6, 2020
Merged

Add HeaderEncodingSelector to MultipartContent #39169

merged 6 commits into from Aug 6, 2020

Conversation

MihaZupan
Copy link
Member

@MihaZupan MihaZupan commented Jul 13, 2020 •
edited

Contributes to #38711

The serialization impl could be optimized to avoid allocations, but I tried to keep the change limited to exposing the API.

Blocked on #39468 atm for the HeaderEncodingSelector<TContext> API

@MihaZupan MihaZupan added this to the 5.0.0 milestone Jul 13, 2020
@MihaZupan MihaZupan requested a review from a team July 13, 2020 01:58
@ghost
Copy link

ghost commented Jul 13, 2020

Tagging subscribers to this area: @dotnet/ncl
Notify danmosemsft if you want to be subscribed.

@Dotnet-GitSync-Bot
Copy link
Collaborator

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ManickaP
ManickaP reviewed Jul 13, 2020
View reviewed changes
src/libraries/System.Net.Http/tests/FunctionalTests/MultipartContentTest.cs Outdated Show resolved Hide resolved
src/libraries/System.Net.Http/tests/FunctionalTests/MultipartContentTest.cs Outdated Show resolved Hide resolved
src/libraries/System.Net.Http/src/System/Net/Http/MultipartContent.cs Outdated
}
}

private static void WriteLatin1ToStream(Stream stream, string content)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Originally, we used HttpRuleParser.DefaultHttpEncoding, now we're using hardcoded Latin1. Is there a reason for the change?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did that here because the method explicitly calls for Latin1 in the name.

I don't believe we would ever change HttpRuleParser.DefaultHttpEncoding to something else due to compat, so I can use it here if we prefer it for consistency.

Copy link
Member

@ManickaP ManickaP Jul 13, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what I'm asking 😄 Why even introduce WriteLatin1ToStream?
I'm aware that the default HTTP encoding will hardly ever change, but the constant exists in our code, probably for a reason, and it's used in other places as well (FormUrlEncodedContent, HeaderDescriptor, HttpConnection).

Btw, the code looks the same as in WriteToStream hot path except the expected array length (IsSingleByte on encoding could help with better stackalloc allocation)

aik-jahoda
aik-jahoda reviewed Jul 13, 2020
View reviewed changes
src/libraries/System.Net.Http/src/System/Net/Http/MultipartContent.cs
@@ -219,12 +220,17 @@ private protected async Task SerializeToStreamAsyncCore(Stream stream, Transport
await EncodeStringToStreamAsync(stream, "--" + _boundary + CrLf, cancellationToken).ConfigureAwait(false);

// Write each nested content.
var output = new StringBuilder();
var output = new MemoryStream();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we write directly into stream to prevent copying?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can at the cost of doubling the SerializeHeaders logic

src/libraries/System.Net.Http/src/System/Net/Http/MultipartContent.cs Outdated Show resolved Hide resolved
@jaredpar jaredpar mentioned this pull request Jul 13, 2020
Closed
scalablecory
scalablecory approved these changes Jul 28, 2020
View reviewed changes
Copy link
Contributor

@scalablecory scalablecory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm once comments addressed.

ManickaP
ManickaP reviewed Jul 28, 2020
View reviewed changes
src/libraries/System.Net.Http/src/System/Net/Http/MultipartContent.cs Outdated
{
Debug.Assert(ReferenceEquals(Encoding.Latin1, HttpRuleParser.DefaultHttpEncoding));

WriteToStream(stream, content, HttpRuleParser.DefaultHttpEncoding);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't we use this exact line instead of WriteLatin1ToStream? Is this helper necessary if it's just a one-liner?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I renamed it to just be an overload of WriteToStream

private static void WriteToStream(Stream stream, string content) =>
    WriteToStream(stream, content, HttpRuleParser.DefaultHttpEncoding);

So that code like this doesn't have to be too verbose in passing in the encoding every time

WriteToStream(stream, CrLf + "--"); // const strings
WriteToStream(stream, _boundary);
WriteToStream(stream, CrLf);

@MihaZupan
Copy link
Member Author

@ManickaP Could you please take another look at the changes here?

Changes since the last time:

  • Updated the PR to use the new HeaderEncodingSelector<TContext> delegate instead of Func
  • Updated the TryComputeLength logic to account for header encodings and added tests for it
  • Changed WriteToStream to rely on Encoding.GetMaxByteCount instead of magic constants
  • Renamed WriteLatin1ToStream to just WriteToStream

ManickaP
ManickaP approved these changes Aug 5, 2020
View reviewed changes
Copy link
Member

@ManickaP ManickaP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks.

@jaredpar jaredpar mentioned this pull request Aug 5, 2020
Closed
@MihaZupan MihaZupan merged commit 53d70b0 into dotnet:master Aug 6, 2020
@MihaZupan MihaZupan mentioned this pull request Aug 6, 2020
Closed
Jacksondr5 pushed a commit to Jacksondr5/runtime that referenced this pull request Aug 10, 2020
@MihaZupan @Jacksondr5
Add HeaderEncodingSelector to MultipartContent (dotnet#39169)
17d80d0 
* Add HeaderEncodingSelector to MultipartContent

* Test cleanup

* Avoid WriteLatin1 logic duplication

* Move to common HeaderEncodingSelector<TContext>

* Fix indentation
@antonfirsov antonfirsov mentioned this pull request Sep 8, 2020
Merged
@dotnet dotnet locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@aik-jahoda aik-jahoda aik-jahoda left review comments

@stephentoub stephentoub stephentoub left review comments

@scalablecory scalablecory scalablecory approved these changes

@ManickaP ManickaP ManickaP approved these changes

Assignees
No one assigned
Labels
area-System.Net.Http new-api-needs-documentation
Projects
None yet
Milestone
5.0.0
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@MihaZupan @Dotnet-GitSync-Bot @aik-jahoda @stephentoub @scalablecory @ManickaP