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

BrowserHttpMessageHandler reads body content to end #5586

Closed
csnewman opened this issue Sep 4, 2018 · 9 comments
Closed

BrowserHttpMessageHandler reads body content to end #5586

csnewman opened this issue Sep 4, 2018 · 9 comments
Labels
area-blazor Includes: Blazor, Razor Components enhancement This issue represents an ask for new feature or an enhancement to an existing one
Milestone

Comments

@csnewman
Copy link

csnewman commented Sep 4, 2018

Inside "Blazor/src/Microsoft.AspNetCore.Blazor.Browser.JS/src/Services/Http.ts", the response data is handled via " responseData = await response.arrayBuffer();", which reads the entire response stream to the end, and then returns the data, however this is the incorrect behaviour.

HttpMessageHandler's should not buffer the response as this is handled within the HttpClient.

This means that method such as "_httpClient.SendAsync(request, HttpCompletionOption.ResponseHeadersRead, CancellationToken.None);", are not handled correctly as the ResponseHeadersRead flag is not respected, due to the content body being read to the end.

We would need to wrap the ReadableStream from the JS side to the c# side as the HttpContent in the ReceiveResponse in BrowserHttpMessageHandler.

Is there any chance of getting this fixed? As I've managed to get the c# signalr client working with long polling, but can't get the "ServerSentEventsTransport" to work until this is fixed, as otherwise the client timeouts trying to read the content body to the end, when it shouldn't as the ResponseHeadersRead is passed.

@Suchiman
Copy link
Contributor

Suchiman commented Sep 4, 2018

I have considered implementing Streaming responses but last time i've looked at it, it was and still is considered experimental https://developer.mozilla.org/en-US/docs/Web/API/Body/body#Browser_compatibility and not supported everywhere.
Aside from that, this should be doable i think.

@csnewman
Copy link
Author

csnewman commented Sep 4, 2018

There are some polyfills available for the streams api, however even without these polyfills, being able to detect and use the correct behaviour when available would be a massive set forward.

For me in my use case, if the browser doesn't support the streams api, I would just disable the server events transport in signalr, and fallback to long polling.

@danroth27
Copy link
Member

Thanks for reporting this issue!

I've managed to get the c# signalr client working with long polling

Is you're main goal here to get a working SignalR client? Have you looked at https://github.com/BlazorExtensions/SignalR?

@csnewman
Copy link
Author

csnewman commented Sep 4, 2018

Yeah, thanks for the suggestion, but I'm working on using the .Net client within Blazor, instead of that wrapped api version.

You can take a look if you want? https://github.com/csnewman/BlazorSignalR, Currently only long polling works.

This approach has the benefit of it just being an addon for the actual .net api, and having all the relevant docs available and supporting all future versions (provided the underlying transport mechanics stay the same). I'll probably end up just implementing the websocket and serversentevent transport in typescript, and proxying those.

@danroth27
Copy link
Member

Got it - makes sense.

@Suchiman
Copy link
Contributor

Suchiman commented Sep 4, 2018

@csnewman i've opened #1387 which makes ResponseHeadersRead work to allow reading headers without reading the body. Does that fix your issue or do you also strictly require that the response body gets streamed as well?

@csnewman
Copy link
Author

csnewman commented Sep 4, 2018

@Suchiman the response body must be streamed for server side events, as the connection is kept open forever, and messages are wrote to the body as available, meaning the body can never be fully read, it must be streamed.

Thanks for creating that fix though, and I imagine it will fix some simpler use cases, however fixing message handler is the only real solution.

@csnewman
Copy link
Author

csnewman commented Sep 5, 2018

@Suchiman There's no rush on fixing this, as I have worked around the lack of support by just proxying the requests across to JS and using the browsers mechanisms (like EventSource).

@aspnet-hello aspnet-hello transferred this issue from dotnet/blazor Dec 17, 2018
@aspnet-hello aspnet-hello added this to the Backlog milestone Dec 17, 2018
@aspnet-hello aspnet-hello added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates enhancement This issue represents an ask for new feature or an enhancement to an existing one area-blazor Includes: Blazor, Razor Components labels Dec 17, 2018
@mkArtakMSFT mkArtakMSFT removed area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates labels May 9, 2019
@pranavkm
Copy link
Contributor

The browser handler in the RTM release of Blazor supports streaming. Consider using it if you want unbuffered responses.

@dotnet dotnet locked as resolved and limited conversation to collaborators Jul 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-blazor Includes: Blazor, Razor Components enhancement This issue represents an ask for new feature or an enhancement to an existing one
Projects
None yet
Development

No branches or pull requests

6 participants