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.

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

Add CancellationToken overloads for reading multipart form sections #41532

Closed
martincostello opened this issue May 5, 2022 · 5 comments · Fixed by #41533
Closed

Add CancellationToken overloads for reading multipart form sections #41532

martincostello opened this issue May 5, 2022 · 5 comments · Fixed by #41533
Labels
api-approved API was approved in API review, it can be implemented area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Milestone

Comments

@martincostello
Copy link
Member

martincostello commented May 5, 2022

Background and Motivation

While checking for other usages of ReadAsStringAsync() when working on #41531, I found one other occurrence here:

public Task<string> GetValueAsync()
{
return Section.ReadAsStringAsync();
}

As well as adding an overload for a CancellationToken parameter there, another would be needed here:

/// <summary>
/// Reads the body of the section as a string
/// </summary>
/// <param name="section">The section to read from</param>
/// <returns>The body steam as string</returns>
public static async Task<string> ReadAsStringAsync(this MultipartSection section)

The new overload could then be used here:

var value = await formDataSection.GetValueAsync();

Proposed API

namespace Microsoft.AspNetCore.WebUtilities;

public class FormMultipartSection
{
+   public Task<string> GetValueAsync(CancellationToken cancellationToken);
}

public static class MultipartSectionStreamExtensions
{
+   public static async Task<string> ReadAsStringAsync(this MultipartSection section, CancellationToken cancellationToken);
}

Usage Examples

var formDataSection = new FormMultipartSection(section, contentDisposition);
var key = formDataSection.Name;
var value = await formDataSection.GetValueAsync(cancellationToken);

Alternative Designs

Add an optional parameter instead as a binary-breaking change for .NET 7.

namespace Microsoft.AspNetCore.WebUtilities;

public class FormMultipartSection
{
-   public Task<string> GetValueAsync();
+   public Task<string> GetValueAsync(CancellationToken cancellationToken = default);
}

public static class MultipartSectionStreamExtensions
{
-   public static async Task<string> ReadAsStringAsync(this MultipartSection section);
+   public static async Task<string> ReadAsStringAsync(this MultipartSection section, CancellationToken cancellationToken = default);
}

Risks

None known.

@martincostello martincostello added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label May 5, 2022
@Tratcher
Copy link
Member

Tratcher commented May 5, 2022

@adityamandaleeka adityamandaleeka added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label May 6, 2022
@ghost
Copy link

ghost commented May 6, 2022

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

  • The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
  • The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
  • Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

@adityamandaleeka adityamandaleeka added this to the .NET 7 Planning milestone May 6, 2022
@ghost
Copy link

ghost commented May 6, 2022

Thanks for contacting us.

We're moving this issue to the .NET 7 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@davidfowl
Copy link
Member

Opportunity for optimization! ValueTask<string> instead of Task<string>

@halter73
Copy link
Member

halter73 commented May 9, 2022

API Review Notes:

  • ValueTask? Yes, for when the data is already buffered. Stream already varies between Task and ValueTask.
  • FileMultipartSection doesn't have a GetValueAsync

Approved:

namespace Microsoft.AspNetCore.WebUtilities;

public class FormMultipartSection
{
+   public ValueTask<string> GetValueAsync(CancellationToken cancellationToken);
}

public static class MultipartSectionStreamExtensions
{
+   public static async ValueTask<string> ReadAsStringAsync(this MultipartSection section, CancellationToken cancellationToken);
}

@halter73 halter73 added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels May 9, 2022
martincostello added a commit to martincostello/aspnetcore that referenced this issue May 10, 2022
Add CancellationToken overloads to methods for reading multipart form sections and use in FormFeature.
Update MultipartSectionStreamExtensions to apply IDE refactoring suggestions.
Relates to dotnet#41532.
@Tratcher Tratcher removed the api-suggestion Early API idea and discussion, it is NOT ready for implementation label May 11, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jun 11, 2022
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants