Add cancellation overloads on HttpClient and HttpContent

[baal2000]

It seems obvious that there is a lack of external cancellation support in a few HttpClient methods: GetByteArrayAsync, GetStreamAsync and maybe others. Any particular reason?

What is expected:

class HttpClient {
    public Task<byte[]> GetByteArrayAsync(string requestUri, CancellationToken token = default);
    public Task<byte[]> GetByteArrayAsync(Uri requestUri, CancellationToken token = default);

    public Task<byte[]> GetStreamAsync(string requestUri, CancellationToken token = default);
    public Task<byte[]> GetStreamAsync(Uri requestUri, CancellationToken token = default);

    public Task<byte[]> GetStringAsync(string requestUri, CancellationToken token = default);
    public Task<byte[]> GetStringAsync(Uri requestUri, CancellationToken token = default);
}
class HttpContent {
    public Task<byte[]> GetByteArrayAsync(CancellationToken token = default);
    public Task<byte[]> GetStreamAsync(CancellationToken token = default);
    public Task<byte[]> GetStringAsync(CancellationToken token = default);
}

I might be not alone here. If missed another case or the feature is in the pipeline already, then comment to the case and close as a duplicate.

Thanks.

P.S.
GetByteArrayAsync is another overload candidate mentioned by dotnet/corefx#32762.

[davidsh]

Duplicate of dotnet/corefx#9071

[davidsh]

Turns out dotnet/corefx#9071 is about HttpContent methods such as ReadAsStringAsync().

This issue is about HttpClient methods specifically.

[dmytro-gokun]

Will .NET 3.0 have these methods?

[stephentoub]

Will .NET 3.0 have these methods?

No

[dmytro-gokun]

@stephentoub Okay, it was a straight answer) Thanks for the update, anyway.

[karelz]

Triage: We agree, we should just add it.

[terrajobst]

Video

• Looks good, but optionality is pointless because the overload without the token already exists
• Name of methods on HttpContent should be ReadAsXxx
• Return types needs fixing up

partial class HttpClient
{
    // New APIs

    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);

    // Existing APIs

    public Task<byte[]> GetByteArrayAsync(string requestUri);
    public Task<byte[]> GetByteArrayAsync(Uri requestUri);

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

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

partial class HttpContent
{
    // New APIs

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

    // Existing APIs

    public Task<byte[]> ReadAsByteArrayAsync();
    public Task<Stream> ReadAsStreamAsync();
    public Task<string> ReadAsStringAsync();
}

[davidfowl]

@terrajobst Any reason we didn't look at ValueTask? Consistency?

[benaadams]

Surely this is an opportunity to get ValueTask overloads in?

Passing a cancellationToken is probably an advanced scenario 😅 so should be ok with the semantics of ValueTask.

Hot-path for it is when Kestrel is used as the internet facing a proxy in front of plethora of microservices, some messages of which will be small and be already completed.

[stephentoub]

Surely this is an opportunity to get ValueTask overloads in?

I can of course see the benefits of these returning ValueTask, the primary way folks consume the result of these methods is with await, etc. But I do want to call out the dangers of making these return ValueTask.

Imagine someone today has:

string result = content.ReadAsStringAsync().GetAwaiter().GetResult();

Now they see we've added support for CancellationToken (yay) and add it to their code:

string result = content.ReadAsStringAsync(cancellationToken).GetAwaiter().GetResult();

Little do they know they now have a bad race condition.

[benaadams]

Hmm, I see your point. Probably would need a different api shape.

(In a similar vein as the Memory overloads on Stream; but starting at HttpClient -> getting the data)

I assume a partial read that you pass a Memory into would be a no go; as that's just replicating the Stream api on HttpClient.

What methods would you suggest?

[MihaZupan]

Is this ready for implementation as-is or should it go back into discussion to consider ValueTask?
@terrajobst @stephentoub

[davidsh]

Is this ready for implementation as-is or should it go back into discussion to consider ValueTask?
@terrajobst @stephentoub

Yes it is ready. The API was approved. We had a lot of internal email discussion on the places where 'ValueTask' should be used. But for APIs like these where we have an existing pattern, we should be consistent.

[MihaZupan]

HttpContent.ReadAsStreamAsync(CancellationToken) is problematic.

When using HttpClient, it either returns the buffered content or it returns the underlying stream i.e. it's not cancelable.

This overload would only make a difference when

1. Using HttpCompletionOption.ResponseHeadersRead
2. Using a custom HttpClientHandler (one not implemented in BCL)
3. That handler returns a custom HttpContent (as far as I can tell, BCL only returns HttpConnectionResponseContent)
4. That content overrides CreateContentReadStreamAsync/SerializeToStreamAsync and actually does async work that can be canceled

Meaning that only unbuffered requests where ReadAsStreamAsync does not return the underlying stream would be affected.

So, option nr. 1 is to just not introduce this overload.

Otherwise, an overload of CreateContentReadStreamAsync that accepts a CT would be needed.

partial class HttpContent
{
    // Existing
    protected virtual Task<Stream> CreateContentReadStreamAsync();

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

There are a few possible implementation options:

1. Have the new overload discard the CT and call the existing one
2. Avoid calling the old overload, thus breaking existing implementations relying on it being called
3. Mark the existing overload as obsolete (I don't think there should be many implementations of it outside of BCL)

The behavior described under 2. is what was decided for SerializeToStreamAsync in dotnet/corefx#9071.

@dotnet/ncl

[davidsh]

HttpContent.ReadAsStreamAsync(CancellationToken) is problematic.

Strictly speaking, the HttpContent.ReadAs* APIs has nothing to do with HttpClient. :)

It is true that in most cases HttpContent objects are obtained as a result of using HttpClient and getting an HttpResponseMessage which has a .Content property of derived type 'HttpContent'. But HttpContent was created as a separate type because it is also an abstraction around content in general such as request-body content.

Many of the built-in HttpContent derived types are simple and are buffer based (StringContent, ByteArrayContent, etc.). But complex streaming based content objects can be created by deriving from the HttpContent abstract class and overriding the CreateContentReadStreamAsync method.

The key problem I see with adding CancellationToken overloads for HttpContent.ReadAs* APIs is based on your 4th statement:

That content overrides CreateContentReadStreamAsync/SerializeToStreamAsync and actually does async work that can be canceled.

So, I would agree that to fix this problem, we do need an additional API overload:

partial class HttpContent
{
    // Existing
    protected virtual Task<Stream> CreateContentReadStreamAsync();

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

Adding a new protected virtual method is ok as long as we add a default implementation. But to get better value from it, we also need to update all of our inbox HttpContent derived types.

[MihaZupan]

I agree that there could be an implementation that would benefit from the overload.

That said, I don't think CreateContentReadStreamAsync is the place where such an implementation should be performing a lot of work - it can always return a custom stream that does work as needed during reads.
Regardless, the API can still be added in case someone decides to do so.

If I understand you correctly, you agree that the API should be added in a way that the default implementation drops the CT (option 2 above)?

[davidsh]

If I understand you correctly, you agree that the API should be added in a way that the default implementation drops the CT (option 2 above)?

Yes, because third-party code that created derived types from HttpContent will not have provided an overload for the new API.

[MihaZupan]

Closing manually as #686 was merged.