Skip to content

Conversation

@ManickaP
Copy link
Member

@ManickaP ManickaP commented Jun 5, 2020

Prototype, it will not compile on master until #34948 is merged.

@scalablecory Could you please give it a look that it's ok before we proceed with an API review process.

cc: @stephentoub

@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.

@ghost
Copy link

ghost commented Jun 5, 2020

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

@ManickaP ManickaP force-pushed the mapichov/sync_http_content_get_stream branch 2 times, most recently from 34c8ad2 to eabed04 Compare June 25, 2020 17:56
@ManickaP ManickaP marked this pull request as ready for review June 25, 2020 18:40
@ManickaP ManickaP changed the title [WIP] HttpContent.ReadAsStream API HttpContent.ReadAsStream API Jun 25, 2020
@ManickaP ManickaP requested a review from a team June 25, 2020 18:40
@ManickaP
Copy link
Member Author

@scalablecory @stephentoub : this is ready for review.
There's one open question from my side in this comment.
I'll plan to add few more tests once this is cleared.

Copy link
Member

Choose a reason for hiding this comment

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

All of these ? and ?? operators makes it hard to understand order of precedence here. Can you please add some parens to help make it clear what's going on?

Copy link
Member Author

Choose a reason for hiding this comment

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

Split up into if/else, defaulting to empty MemoryStream moved as the last step.

@ManickaP ManickaP force-pushed the mapichov/sync_http_content_get_stream branch from 8ffb324 to 028567b Compare June 26, 2020 13:03
@ManickaP
Copy link
Member Author

/azp run runtime-libraries outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ManickaP ManickaP force-pushed the mapichov/sync_http_content_get_stream branch from 8d4393e to 01f59c0 Compare June 27, 2020 19:32
@stephentoub
Copy link
Member

There are a couple of places in the implementation of SocketsHttpHandler that should be updated to use the synchronous ReadAsStream (because ReadAsStreamAsync will always complete synchronously but we pay an additional task object to use it):

Stream stream = await response.Content.ReadAsStreamAsync(cancellationToken).ConfigureAwait(false);

return (await tunnelResponse.Content!.ReadAsStreamAsync(cancellationToken).ConfigureAwait(false), null);

@ManickaP
Copy link
Member Author

There are a couple of places in the implementation of SocketsHttpHandler that should be updated to use the synchronous ReadAsStream (because ReadAsStreamAsync will always complete synchronously but we pay an additional task object to use it):

Fixed

@ManickaP
Copy link
Member Author

CI's finished and green, it just doesn't show here yet.

@ManickaP ManickaP merged commit 7d9ec59 into dotnet:master Jun 29, 2020
@ManickaP ManickaP deleted the mapichov/sync_http_content_get_stream branch June 29, 2020 11:55
ManickaP added a commit to ManickaP/runtime that referenced this pull request Jul 2, 2020
@ManickaP ManickaP mentioned this pull request Jul 2, 2020
@karelz karelz added this to the 5.0.0 milestone Aug 18, 2020
@ghost ghost 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants