Skip to content

Fix NET 7.0 streamed http response reading #80638

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

Closed
wants to merge 1 commit into from
Closed

Fix NET 7.0 streamed http response reading #80638

wants to merge 1 commit into from

Conversation

rabirland
Copy link

The JS stream in the NET 7.0 Blazor runtime blocks until either the stream has ended, or the C# buffer is full, preventing the developer from reading the responses as soon as they are ready. A workaround exists by limiting the read buffer size, but it prevents the usage of StreamReaders and still causes the StreamContent.ReadAsync() to not return with a zero.

The endlessly running await StreamContent.ReadAsync(...) also interferes with the other asynchronous operations, causing them to halt as well.

Fix for: #79238

Known issues:

  • As in the original implementation, ReadableStreamDefaultReader.read() also blocks the execution if there is no data available, and currently there is no known API to check if there are bytes received or not.

@ghost ghost added area-System.Runtime.InteropServices.JavaScript community-contribution Indicates that the PR has been added by a community member labels Jan 13, 2023
@dnfadmin
Copy link

dnfadmin commented Jan 13, 2023

CLA assistant check
All CLA requirements met.

@JamesNK
Copy link
Member

JamesNK commented Jan 14, 2023

I think some testing is needed for this feature to prevent future regressions.

@pavelsavara pavelsavara self-assigned this Jan 16, 2023
@pavelsavara pavelsavara added this to the 8.0.0 milestone Jan 16, 2023
@pavelsavara pavelsavara added the arch-wasm WebAssembly architecture label Jan 16, 2023
@ghost
Copy link

ghost commented Jan 16, 2023

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

The JS stream in the NET 7.0 Blazor runtime blocks until either the stream has ended, or the C# buffer is full, preventing the developer from reading the responses as soon as they are ready. A workaround exists by limiting the read buffer size, but it prevents the usage of StreamReaders and still causes the StreamContent.ReadAsync() to not return with a zero.

The endlessly running await StreamContent.ReadAsync(...) also interferes with the other asynchronous operations, causing them to halt as well.

Fix for: #79238

Known issues:

  • As in the original implementation, ReadableStreamDefaultReader.read() also blocks the execution if there is no data available, and currently there is no known API to check if there are bytes received or not.
Author: rabirland
Assignees: pavelsavara
Labels:

arch-wasm, area-System.Runtime.InteropServices.JavaScript, community-contribution

Milestone: 8.0.0

@pavelsavara
Copy link
Member

Waiting until C# buffer is full is wrong in the current code, thanks for figuring it out @rabirland !

I will open my own PR for fixing it, together with another issue and also try to create unit test for it.

if (remaining_source === 0) {
res.__chunk = await res.__reader.read();
res.__source_offset = 0;
return 0;
Copy link
Member

@pavelsavara pavelsavara Jan 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that we could not return zero here as it means end of stream on C# side.
https://learn.microsoft.com/en-us/dotnet/api/system.io.stream.readasync?view=net-7.0

I also think we rather keep the loop over __reader.read(); in case that browser returned zero length chunk.
I worry it may happen when server sends zero sized chunk.

I think we need to block the promise/task until we receive at least one byte or until we get done.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to block the promise/task until we receive at least one byte or until we get done.

Yes, unless zero bytes were requested (in which case you can either return immediately or wait for at least one byte to be available but not consume it), Stream.Read must not return 0 unless it's EOF, and Stream.ReadAsync must not complete the task with 0 unless it's EOF.

Copy link
Author

@rabirland rabirland Jan 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Waiting until C# buffer is full is wrong in the current code, thanks for figuring it out @rabirland !

You're welcome. To remove the zero return, you gotta bridge the if (!res.body) check. After that, res__reader.read() will block until data is received.

The if (remaining_source === 0) need to re-run the chunk read, instead of returning and waiting for the C# side to call it again.

@pavelsavara
Copy link
Member

[00:57:20] fail: [FAIL] System.Net.Http.Functional.Tests.SocketsHttpHandler_HttpClientHandlerTest.ReadAsStreamAsync_HandlerProducesWellBehavedResponseStream(chunked: True, enableWasmStreaming: True)
[00:57:20] info: Assert.Equal() Failure
[00:57:20] info: Expected: 101
[00:57:20] info: Actual:   0

Log https://helixre107v0xdeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-80638-merge-da0dddf4d6fa48fa8f/WasmTestOnBrowser-System.Net.Http.Functional.Tests/1/console.181e1bb5.log?helixlogtype=result

@pavelsavara
Copy link
Member

I'm working on it here #80693

@ghost ghost locked as resolved and limited conversation to collaborators Feb 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-System.Runtime.InteropServices.JavaScript community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants