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 HttpResponse APIs (ClearHeaders/WriteFile/TransmitFile/etc) #102

Merged
merged 9 commits into from
Jul 13, 2022

Conversation

twsouthwick
Copy link
Member

@twsouthwick twsouthwick commented Jul 11, 2022

This change adds a few APIs that were missing from HttpResponse.

SubStatusCode is a placeholder for the value, but does not actually publish it anywhere.

TransmitFile/WriteFile: These APIs are essentially the same, except that they had different memory characteristics. TransmitFile did not buffer, while WriteFile did. On ASP.NET Core, these will just delegate to IHttpResponseBodyFeature.SendFileAsync.

NOTE: This implementation will call Task.GetAwaiter() which should be avoided. We could potentially expose an async API for this on the ASP.NET Core side as a stepping stone if we want.

Part of #91

@twsouthwick twsouthwick force-pushed the tasou/httpresponse-writefile branch 3 times, most recently from f47cc91 to 98b90ce Compare July 11, 2022 17:36
These APIs are essentially the same, except that they had different memory characteristics. TransmitFile did not buffer, while WriteFile did. On ASP.NET Core, these will just delegate to IHttpResponseBodyFeature.SendFileAsync.

NOTE: This implementation will call Task.GetAwaiter().GetResult(). We could potentially expose an async API for this on the ASP.NET Core side as a stepping stone if we want.

Part of #91
@twsouthwick twsouthwick changed the title Add HttpResponse.WriteFile/TransmitFile Add HttpResponse APIs (ClearHeaders/WriteFile/TransmitFile/etc) Jul 11, 2022
Copy link
Member

@mjrousos mjrousos left a comment

Choose a reason for hiding this comment

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

Looks good other than a question about SubStatusCode's initial value

Copy link
Member

@mjrousos mjrousos left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants