Skip to content

Get rid of StreamWriter usage in HTTP loopback server and fix HTTP/1.1 loopback implementation of SendResponseBodyAsync#47364

Merged
geoffkizer merged 10 commits intodotnet:masterfrom
geoffkizer:loopbackcleanup
Jan 26, 2021
Merged

Get rid of StreamWriter usage in HTTP loopback server and fix HTTP/1.1 loopback implementation of SendResponseBodyAsync#47364
geoffkizer merged 10 commits intodotnet:masterfrom
geoffkizer:loopbackcleanup

Conversation

@geoffkizer
Copy link
Contributor

@geoffkizer geoffkizer commented Jan 23, 2021 •
edited
Loading

The StreamWriter usage is not needed, and the incorrect implementation of SendResponseBodyAsync is problematic.

@wfurt @dotnet/ncl

@ghost ghost added the area-System.Net.Http label Jan 23, 2021
@ghost
Copy link

ghost commented Jan 23, 2021

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

This is not needed, and causes some confusion in places in the tests.

@wfurt @dotnet/ncl

Author: geoffkizer
Assignees: -
Labels:

area-System.Net.Http
Milestone: -

@geoffkizer geoffkizer changed the title Get rid of StreamWriter usage in HTTP loopback server Get rid of StreamWriter usage in HTTP loopback server and fix HTTP/1.1 loopback implementation of SendResponseBodyAsync Jan 23, 2021
stephentoub
stephentoub approved these changes Jan 25, 2021
View reviewed changes
Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

Other than the compilation failure, LGTM.

wfurt
wfurt approved these changes Jan 25, 2021
View reviewed changes
Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

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

LGTM. I think it is better to have string overload than depend on the writer. We'll just need to clean up remains references,

@wfurt
Copy link
Member

wfurt commented Jan 25, 2021

BTW I don't see past failures of SendAsync_WithZeroLengthHeaderName_Throws. So it may be related to the PR

@geoffkizer
Copy link
Contributor Author

geoffkizer commented Jan 25, 2021

BTW I don't see past failures of SendAsync_WithZeroLengthHeaderName_Throws. So it may be related to the PR

Yeah I think this PR expose a timing-related exception here. Pushed a fix for this.

@runfoapp runfoapp bot mentioned this pull request Jan 25, 2021
Closed
@geoffkizer geoffkizer merged commit 59417ef into dotnet:master Jan 26, 2021
@geoffkizer geoffkizer deleted the loopbackcleanup branch January 26, 2021 00:31
@karelz karelz added this to the 6.0.0 milestone Jan 26, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Feb 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@stephentoub stephentoub stephentoub approved these changes

@wfurt wfurt wfurt approved these changes

Assignees

No one assigned

Labels

area-System.Net.Http

Projects

None yet

Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@geoffkizer @wfurt @stephentoub @karelz