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.

Sign up for GitHub

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

Jump to bottom

[browser] http streaming request server error #105709

Merged
merged 2 commits into from
Aug 6, 2024
Merged

[browser] http streaming request server error #105709

merged 2 commits into from
Aug 6, 2024

Conversation

pavelsavara
Copy link
Member

@pavelsavara pavelsavara commented Jul 30, 2024 •
edited
Loading

  • propagate exceptions from http_wasm_transform_stream_write and http_wasm_transform_stream_close as BrowserHttpWriteStream.Rejected
  • improves StatusCode.ashx test helper to be able to delay
  • unified AbortRequest and AbortResponse into BrowserHttpInterop.AbortResponse
  • unified http_wasm_abort_request and http_wasm_abort_response into http_wasm_abort
  • added handler handle_abort_error
    • only use controller.abortController when there is no streamWriter or streamReader
  • removed chrome workaround for abort
  • enabled BrowserHttpHandler_StreamingRequest_CancelRequest on CI

Fixes #104612

@pavelsavara pavelsavara added arch-wasm WebAssembly architecture area-System.Runtime.InteropServices.JavaScript os-browser Browser variant of arch-wasm labels Jul 30, 2024
@pavelsavara pavelsavara added this to the 9.0.0 milestone Jul 30, 2024
@pavelsavara pavelsavara requested a review from maraf July 30, 2024 16:31
@pavelsavara pavelsavara self-assigned this Jul 30, 2024
@pavelsavara
Copy link
Member Author

cc @campersau , review welcome

it seems that https://bugs.chromium.org/p/chromium/issues/detail?id=1480250 was fixed
do you think we need to keep the workaround for the cancellation in http_wasm_transform_stream_write for the Net9 release ?

I guess it does no harm (with this PR) and we could delete it in Net10

This was referenced Jul 30, 2024
Closed
Open
campersau
campersau reviewed Jul 30, 2024
View reviewed changes
Copy link
Contributor

@campersau campersau left a comment

Choose a reason for hiding this comment

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

Yes, if the tests are passing without the fetch race workaround we could remove this logic.

src/libraries/System.Net.Http/src/System/Net/Http/BrowserHttpHandler/BrowserHttpHandler.cs Show resolved Hide resolved
This was referenced Jul 30, 2024
Open
Open
Open
Closed
Open
Closed
Open
Open
@pavelsavara pavelsavara force-pushed the browser_http_streaming_request_server_error branch from 72f0183 to 6e1d285 Compare August 6, 2024 14:21
This was referenced Aug 6, 2024
Open
Closed
@pavelsavara pavelsavara marked this pull request as ready for review August 6, 2024 17:04
@lewing lewing merged commit edbb2ba into dotnet:main Aug 6, 2024
93 of 95 checks passed
@pavelsavara pavelsavara deleted the browser_http_streaming_request_server_error branch September 2, 2024 15:28
@github-actions github-actions bot locked and limited conversation to collaborators Oct 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@campersau campersau campersau left review comments

@lewing lewing lewing approved these changes

@maraf maraf Awaiting requested review from maraf

Assignees

@pavelsavara pavelsavara

Labels
arch-wasm WebAssembly architecture area-System.Runtime.InteropServices.JavaScript os-browser Browser variant of arch-wasm
Projects
None yet
Milestone
9.0.0
3 participants
@pavelsavara @lewing @campersau