Runtime: HttpContent.ReadAsStream sync API

[ManickaP]

Background and Motivation

We introduced HttpClient sync API in https://github.com/dotnet/runtime/pull/34948. However, the access to response's HttpContent is still purely asynchronous (e.g. ReadAsStreamAsync). To fully support fully a synchronous flow we should also support synchronous read of HttpContent.

Proposed API

Following minimalist approach from https://github.com/dotnet/runtime/issues/32125 we should add sychronous method to retrieve Stream from HttpContent.

namespace System.Net.Http
{
    public abstract partial class HttpContent : System.IDisposable
    {
        // Existing methods:
        protected virtual System.Threading.Tasks.Task<System.IO.Stream> CreateContentReadStreamAsync();
        protected virtual System.Threading.Tasks.Task<System.IO.Stream> CreateContentReadStreamAsync(System.Threading.CancellationToken cancellationToken);

        public System.Threading.Tasks.Task<System.IO.Stream> ReadAsStreamAsync();
        public System.Threading.Tasks.Task<System.IO.Stream> ReadAsStreamAsync(System.Threading.CancellationToken cancellationToken);

        // Proposed new methods, following the same implementation pattern as ReadAsStreamAsync and CreateContentReadStreamAsync        
+       protected virtual System.IO.Stream CreateContentReadStream(System.Threading.CancellationToken cancellationToken = default);

+       public System.IO.Stream ReadAsStream();
+       public System.IO.Stream ReadAsStream(System.Threading.CancellationToken cancellationToken);
     }
}

Implementation Notes

All our own HttpContent implementations do have Stream readily available, see implementations of: https://github.com/dotnet/runtime/blob/cd8355bd2aafff1776df9e009940d91697752152/src/libraries/System.Net.Http/src/System/Net/Http/HttpContent.cs#L475

Discussion Topics

• CancellationToken support in ReadAsStream (include or not)?
• Include CreateContentReadStream both overloads, one overload with default, or only one overload without default?

Prototype in https://github.com/dotnet/runtime/pull/37494

cc: @dotnet/ncl @stephentoub

[msftbot[bot]]

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

[karelz]

@ManickaP is there a typo? Should first 2 ReasAsStream be ReadAsStreamAsync?

CreateContentReadStream - we should not have 2 overloads IMO, where we can, we use default values (we should add it into the API proposal).

CancellationToken support in sync APIs -- we had discussion about it earlier, right? We should follow the same pattern - if we have other Networking sync APIs with CancellationToken on HttpClient and friends, we should do the same here IMO.

[ManickaP]

Should first 2 ReasAsStream be ReadAsStreamAsync?

Yes, I copy-pasted wrong lines. Thanks and fixed.

we should not have 2 overloads IMO, where we can, we use default values

We have a precedent of preferring overloads to optional parameters if there is an existing, similar API on the same class. For instance, 3 Send overloads on HttpClient following SendAsync overloads. However this is a protected virtual method, not a callable, public API of the class, so I agree with you on this.

if we have other Networking sync APIs with CancellationToken on HttpClient and friends, we should do the same here IMO.

I agree with you here as well. I'd prefer to include CancellationToken

I'll update the proposal to reflect all of this.

[scalablecory]

These APIs look good to me.

[karelz]

I guess, we can make also ReadAsStream with default value and collapse the 2 overloads.

[scalablecory]

I guess, we can make also ReadAsStream with default value and collapse the 2 overloads.

In previous API review for sync APIs, we decided against this. Reason being that existing HttpClient APIs don't do this, so keeping them split is more consistent.

If we have strong opinions to use defaulted values here, lets gather rationale that we can bring to API review.

[karelz]

In that case, let's split both of them then ... we should follow our normal guidance IMO

Definitely no strong opinions here ;)

[davidsh]

Consistency with existing APIs is a good thing.

[ManickaP]

Implemented in https://github.com/dotnet/runtime/pull/37494