Skip to content
This repository has been archived by the owner on Jul 9, 2024. It is now read-only.

Streams returned from the HttpClientRequestAdapter cannot be used #10

Closed
calebkiage opened this issue Apr 6, 2022 · 3 comments · Fixed by #11
Closed

Streams returned from the HttpClientRequestAdapter cannot be used #10

calebkiage opened this issue Apr 6, 2022 · 3 comments · Fixed by #11
Assignees

Comments

@calebkiage
Copy link
Contributor

Streams returned from HttpClientRequestAdapter methods like SendPrimitiveAsync<Stream> cannot be used. The methods return disposed streams. I believe the culprit is the new DrainAsync method.

@baywet
Copy link
Member

baywet commented Apr 6, 2022

Ah this is probably a shortcoming on my side when I implemented this. The fact that it's a finally means it always runs the drain async.

await DrainAsync(response);

We probably have avenues opportunities here:

  • Clone the stream into a memory stream, and return that memory stream instead. This might lead to higher memory usage.
  • Add a condition in the finally. Consumers might lock the connections if they don't dispose the stream in that case.

Thoughts?

@andrueastman
Copy link
Member

I believe there are scenarios where the response content may be large and consumers would not want/accept to load the entire stream into memory in some scenarios. On a side note, we should also probably add support for passing the HttpCompletionOption to the httpClient as well.

I think adding a condition for streams should be okay as we already call ReadAsStreamAsync() here for streams so the connection should be released as we have read the response body.

return (ModelType)(await response.Content.ReadAsStreamAsync() as object);

@baywet
Copy link
Member

baywet commented Apr 6, 2022

Makes sense, thanks for the input. I'm not sure what would be the impact for the completion options (worried about side effects on the service side). Would one of you be so kind to submit a PR to add the condition to the finally please?

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

Successfully merging a pull request may close this issue.

3 participants