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

[browser][http] Fix blocking of streaming response and abort #80693

Merged
merged 9 commits into from
Jan 19, 2023

Conversation

pavelsavara
Copy link
Member

@pavelsavara pavelsavara commented Jan 16, 2023

  • return bytes of streaming response as soon as available
  • fix unhandled error in reader.cancel() promise
  • return cancelable promise from http_wasm_get_streamed_response_bytes
  • unit test for slowly streamed chunks
  • unit test for streaming and default cancellation

Fixes #79238
Fixes #80696

Should be merged back to Net7

- return bytes of streaming response as soon as available
@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
  • return bytes of streaming response as soon as available
  • fix unhandled error in reader.cancel() promise

Fixes #79238
Fixes #80696

Author: pavelsavara
Assignees: pavelsavara
Labels:

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

Milestone: 8.0.0

@pavelsavara pavelsavara changed the title [browser][http] Fix bloking of streaming response and abort [browser][http] Fix blocking of streaming response and abort Jan 16, 2023
@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara pavelsavara marked this pull request as ready for review January 16, 2023 20:41
Copy link
Member

@maraf maraf left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@ilonatommy ilonatommy left a comment

Choose a reason for hiding this comment

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

👍 I don't have any more ideas for improvement.

@pavelsavara
Copy link
Member Author

@hakenr, would you please have time to test it ?

@hakenr
Copy link

hakenr commented Jan 18, 2023

@pavelsavara Yes, I'm happy to test it, I just don't know how to get the new version into my own project. Yes, I'm happy to test it, I just don't know how to get the new version into my own project. Can you please direct me where I can find some instructions?

@pavelsavara
Copy link
Member Author

Can you please direct me where I can find some instructions?

I will have to figure it out. My naive attempt to use net8 runtime in net7 blazor project didn't work.
Maybe it would be easier for me to backport it to net7 before we test it with old blazor.

There are NodeJS CI failures which I need to handle first.

Will ping you again, thanks!

@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara pavelsavara merged commit 03db03f into dotnet:main Jan 19, 2023
@pavelsavara
Copy link
Member Author

/backport to release/7.0

@github-actions
Copy link
Contributor

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/3960939369

@github-actions
Copy link
Contributor

@pavelsavara backporting to release/7.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: - fix unhandled error in reader.cancel() promise
Applying: free reader
Applying: fix + test
Applying: test
Applying: test
Applying: feedback
Applying: new test for streaming abort
error: sha1 information is lacking or useless (src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.cs).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0007 new test for streaming abort
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

@github-actions
Copy link
Contributor

@pavelsavara an error occurred while backporting to release/7.0, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

mdh1418 pushed a commit to mdh1418/runtime that referenced this pull request Jan 24, 2023
…80693)

- return bytes of streaming response as soon as available
- fix unhandled error in reader.cancel() promise
- return cancelable promise from http_wasm_get_streamed_response_bytes
- unit test for slowly streamed chunks
- unit test for streaming and default cancellation
pavelsavara added a commit to pavelsavara/runtime that referenced this pull request Jan 24, 2023
- return bytes of streaming response as soon as available
- fix unhandled error in reader.cancel() promise
- return cancelable promise from http_wasm_get_streamed_response_bytes
- unit test for slowly streamed chunks
- unit test for streaming and default cancellation
carlossanlop pushed a commit that referenced this pull request Feb 9, 2023
- return bytes of streaming response as soon as available
- fix unhandled error in reader.cancel() promise
- return cancelable promise from http_wasm_get_streamed_response_bytes
- unit test for slowly streamed chunks
- unit test for streaming and default cancellation
@ghost ghost locked as resolved and limited conversation to collaborators Feb 18, 2023
@pavelsavara pavelsavara deleted the browser_http_stream branch September 2, 2024 15:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants