Skip to content

Conversation

@MihaZupan
Copy link
Member

@MihaZupan MihaZupan commented Dec 9, 2019

Closes #916 (dotnet/corefx#32615), closes #576 and closes #17492 (dotnet/corefx#9071).

This PR implements the following APIs:

partial class HttpClient
{
    public Task<byte[]> GetByteArrayAsync(string requestUri, CancellationToken cancellationToken);
    public Task<byte[]> GetByteArrayAsync(Uri requestUri, CancellationToken cancellationToken);

    public Task<Stream> GetStreamAsync(string requestUri, CancellationToken cancellationToken);
    public Task<Stream> GetStreamAsync(Uri requestUri, CancellationToken cancellationToken);

    public Task<string> GetStringAsync(string requestUri, CancellationToken cancellationToken);
    public Task<string> GetStringAsync(Uri requestUri, CancellationToken cancellationToken);
}

partial class HttpContent
{
    public Task<byte[]> ReadAsByteArrayAsync(CancellationToken cancellationToken);
    public Task<Stream> ReadAsStreamAsync(CancellationToken cancellationToken);
    public Task<string> ReadAsStringAsync(CancellationToken cancellationToken);

    protected virtual Task<Stream> CreateContentReadStreamAsync(CancellationToken cancellationToken);

    protected virtual Task SerializeToStreamAsync(Stream stream, TransportContext context, CancellationToken cancellationToken);

    public Task CopyToAsync(Stream stream, CancellationToken cancellationToken);
    public Task CopyToAsync(Stream stream, TransportContext context, CancellationToken cancellationToken);
}

Implements APIs approved in dotnet/corefx#32615 and dotnet#576
@MihaZupan MihaZupan added the blocked Issue/PR is blocked on something - see comments label Dec 9, 2019
@MihaZupan MihaZupan marked this pull request as ready for review December 9, 2019 14:48
@MihaZupan MihaZupan requested a review from a team December 9, 2019 14:48
@stephentoub
Copy link
Member

stephentoub commented Dec 9, 2019

Blocked by #576, awaiting approval.

We prefer to not open PRs for new APIs until the the APIs have been approved. Can you make the relevant APIs internal until that issue is reviewed/approved? That way the ones that have been approved can proceed in this PR.

@MihaZupan
Copy link
Member Author

MihaZupan commented Dec 9, 2019

We prefer to not open PRs for new APIs until the the APIs have been approved.

I was advised that I should mark the PR as blocked in the case it's only adding CTs to existing overloads.

Can you make the relevant APIs internal until that issue is reviewed/approved? That way the ones that have been approved can proceed in this PR.

They are up for review tomorrow, I assume the PR review will take longer than that, otherwise I can make them internal as you suggested.

@stephentoub
Copy link
Member

I was advised that I should mark the PR as blocked

Thanks. By whom?

@MihaZupan
Copy link
Member Author

By @karelz during the last sync meeting. That was only referring to the need of

protected virtual Task<Stream> CreateContentReadStreamAsync(CancellationToken cancellationToken);

as described in https://github.com/dotnet/corefx/issues/32615#issuecomment-562083237.

Cory asked me to add the CopyToAsync overloads to the same API proposal issue, but I don't think those should be a contentious topic.

@MihaZupan MihaZupan removed the blocked Issue/PR is blocked on something - see comments label Dec 10, 2019
@MihaZupan
Copy link
Member Author

/azp run runtime-libraries outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@dotnet dotnet deleted a comment from azure-pipelines bot Dec 11, 2019
@davidsh davidsh added this to the 5.0 milestone Dec 11, 2019
@MihaZupan
Copy link
Member Author

/azp run runtime-libraries outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MihaZupan MihaZupan requested review from a team, davidsh and stephentoub December 13, 2019 17:01
requestStream, state.TransportContext
#endif
).ConfigureAwait(false);
await state.RequestMessage.Content.CopyToAsync(requestStream, state.TransportContext).ConfigureAwait(false);
Copy link
Member

Choose a reason for hiding this comment

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

If we wanted to we could add a configuration to the System.Net.Http.WinHttpHandler package that would contain a build for .NET 5, and then we could utilize the new APIs in that build. But maybe it's not worth it.

Copy link
Contributor

Choose a reason for hiding this comment

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

It would only help in scenarios where someone wanted to cancel an operation that uploaded request body content (i.e. POST).

I think we can wait for customer feedback and then decide if it is worth adding that ifdef'd code and additional .NET 5 build for the Nuget package.

@MihaZupan
Copy link
Member Author

/azp run runtime-libraries outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

6 participants