Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The response stream from AdvancedResponseWriter is disposed too early #2072

Closed
bednar opened this issue Apr 27, 2023 · 6 comments
Closed

The response stream from AdvancedResponseWriter is disposed too early #2072

bednar opened this issue Apr 27, 2023 · 6 comments

Comments

@bednar
Copy link
Contributor

bednar commented Apr 27, 2023

Describe the bug
I want to stream response data to my API for advanced processing. For this purpose I use AdvancedResponseWriter because I also want to handle errors from remote resource (=> I cannot use RestSharp.RestClient.DownloadStreamAsync), but the latest changes close response stream too early. It is cause by using in:

using var internalResponse = await ExecuteRequestAsync(request, cancellationToken).ConfigureAwait(false);

To Reproduce

[Test]
public async Task AdvancedResponseWriterKeepStream()
{
    await foreach (var line in MyAsyncApi())
    {
        Console.WriteLine(line);
    }
}

private async IAsyncEnumerable<string> MyAsyncApi()
{
    using var client = new RestClient(new RestClientOptions("https://demo.ckan.org/"));
    var restRequest =
        new RestRequest(
            "dataset/c322307a-b871-44fe-a602-32ee8437ff04/resource/b53c9e72-6b59-4cda-8c0c-7d6a51dad12a/download/sample.csv");

    Stream stream = null;
    restRequest.AdvancedResponseWriter = (response, request) =>
    {
        if (!response.IsSuccessStatusCode)
        {
            throw new Exception($"Do something with unsuccessful response: {response}");
        }

        stream = response.Content.ReadAsStreamAsync().ConfigureAwait(false).GetAwaiter().GetResult();

        return new RestResponse(request);
    };

    await client.ExecuteAsync(restRequest);

    using var reader = new StreamReader(stream, Encoding.UTF8);
    while (reader.Peek() >= 0)
    {
        // advanced processing
        yield return await reader.ReadLineAsync();
    }
}

Expected behavior
Be able to stream response to API.

Stack trace

System.ArgumentException : Stream was not readable.
   at System.IO.StreamReader..ctor(Stream stream, Encoding encoding, Boolean detectEncodingFromByteOrderMarks, Int32 bufferSize, Boolean leaveOpen)
   at System.IO.StreamReader..ctor(Stream stream, Encoding encoding)

Desktop (please complete the following information):

  • OS: macOS
  • .NET version: NET 5
  • Version: 110.2.0
@alexeyzimarev
Copy link
Member

alexeyzimarev commented Aug 19, 2023

I think you are using the advanced response writer for the wrong purpose. It's meant to rewrite the response, and it should finish within the scope of one request, before the response is returned. The stream you get as an input should not be used outside of that closure.

You might want to use DownloadStreamAsync or StreamJsonAsync instead.

@alexeyzimarev alexeyzimarev removed the bug label Aug 19, 2023
@sensslen
Copy link
Contributor

sensslen commented Aug 19, 2023

My use case is to inspect the headers before executing the actual download. How would this be possible with DownloadStreamAsync?

@alexeyzimarev
Copy link
Member

You can create a function based on DownloadStreamAsync code. As you can see, it sets the request completion option to ResponseHeadersRead, so you get the headers in the response, and then you can inspect the headers and decide what to do:

    public async Task<Stream?> DownloadStreamAsync(RestRequest request, CancellationToken cancellationToken = default) {
        // Make sure we only read the headers so we can stream the content body efficiently
        request.CompletionOption = HttpCompletionOption.ResponseHeadersRead;
        var response = await ExecuteRequestAsync(request, cancellationToken).ConfigureAwait(false);

        var exception = response.Exception ?? response.ResponseMessage?.MaybeException();

        if (exception != null) {
            return Options.ThrowOnAnyError ? throw exception : null;
        }

        if (response.ResponseMessage == null) return null;

        return await response.ResponseMessage.ReadResponseStream(request.ResponseWriter, cancellationToken).ConfigureAwait(false);
    }

@alexeyzimarev
Copy link
Member

alexeyzimarev commented Aug 19, 2023

Ah sorry, ExecuteRequestAsync is private.

You still can use OnAfterRequest, you still need to set the completion request option to HttpCompletionOption.ResponseHeadersRead:

    /// <summary>
    /// When supplied, the function will be called after the request is complete
    /// </summary>
    public Func<HttpResponseMessage, ValueTask>? OnAfterRequest { get; set; }

@alexeyzimarev
Copy link
Member

My current thought is to change the interface signature and make Download work differently. Right now, it returns Task<Stream?>, which prevents the user to inspect the response for errors. It crashes if the call wasn't successful.

So, the idea is to make ExecuteStreamAsync, which will return Task<RestStreamResponse>. The response will be like RestResponse but instead of Content it will have Stream?. Naturally, it requires to be IDisposable.

The first step there was to create a source generator to create instance of RestResponseBase child class, as copying those properties manually is error prone (adding a new prop requires many changes). That part is done, so now this thing should be relatively easy.

@alexeyzimarev
Copy link
Member

Continues in #2226

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants