-
Notifications
You must be signed in to change notification settings - Fork 13
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
Implement StreamResponse
#110
Conversation
f977a9b
to
978cfb1
Compare
978cfb1
to
234765e
Compare
@@ -165,6 +169,10 @@ private async ValueTask<TResponse> RequestCoreAsync<TResponse>(bool isAsync, Req | |||
response = requestData.ConnectionSettings.ProductRegistration.ResponseBuilder.ToResponse<TResponse> | |||
(requestData, ex, statusCode, responseHeaders, responseStream, mimeType, contentLength, threadPoolStats, tcpStats); | |||
|
|||
// Defer disposal of the response message | |||
if (response is StreamResponse sr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugly but i think its better to pass receive
(as IDisposable) to ResponseBuilder.ToResponse
. To ensure we inject to to StreamResponse
's constructor.
If an exception happens in ToResponse()
we might never set the finalizer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fine for me as well! The whole thing is a little bit "spaghetti" unfortunately due to the fact that the HttpRequestInvoker
must know about StreamResponse
etc. Will change this part later 🙂
@@ -118,6 +118,7 @@ private async ValueTask<TResponse> RequestCoreAsync<TResponse>(bool isAsync, Req | |||
responseMessage = client.SendAsync(requestMessage, HttpCompletionOption.ResponseHeadersRead, cancellationToken).GetAwaiter().GetResult(); | |||
#endif | |||
|
|||
receive = responseMessage; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a single comment to discuss but mostly LGTM!
Implements the
StreamResponse
class that provides direct access to the response content stream.