Skip to content

[browser][http] mute JS exceptions about network errors + HEAD verb - #113014

Merged
pavelsavara merged 19 commits into
dotnet:mainfrom
pavelsavara:browser_http_mute_abort
Mar 7, 2025
Merged

[browser][http] mute JS exceptions about network errors + HEAD verb#113014
pavelsavara merged 19 commits into
dotnet:mainfrom
pavelsavara:browser_http_mute_abort

Conversation

@pavelsavara

@pavelsavarapavelsavara commented Feb 28, 2025•
edited
Loading

Copy link
Copy Markdown
Member

  1. In browser HTTP client we subscribe for rejected promises in order to suppress unhandled rejection of AbortError when we issue the abort. The current handler also handles other network errors and aborts the whole program, which is not what it should do. Instead the network error should be propagated to managed HTTP client code.

  2. in Firefox when the verb is HEAD, the body is null. We should not fail because of that.

  3. Passing Response as result of http_wasm_fetch is creating JS memory leak by marshaling the object.

Contributes to #112172
Contributes to #111992

This bug fix should be back ported.

@pavelsavarapavelsavara added arch-wasm WebAssembly architecture area-System.Net.Http os-browser Browser variant of arch-wasm labels Feb 28, 2025
@pavelsavarapavelsavara added this to the 10.0.0 milestone Feb 28, 2025
@pavelsavarapavelsavara self-assigned this Feb 28, 2025
@dotnet-policy-service

dotnet-policy-serviceBot commented Feb 28, 2025

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

campersau
campersau reviewed Feb 28, 2025
View reviewed changes
Comment threadsrc/mono/browser/runtime/http.ts Outdated
@pavelsavarapavelsavara changed the title [browser][http] mute JS exceptions about network errors[browser][http] mute JS exceptions about network errors + HEAD verbFeb 28, 2025
This was referenced Feb 28, 2025
Closed
Closed
Closed
@build-analysisbuild-analysisBot mentioned this pull request Feb 28, 2025
Closed
3 tasks
This was referenced Mar 3, 2025
Closed
Open
campersau
campersau reviewed Mar 6, 2025
View reviewed changes
Comment threadsrc/mono/browser/runtime/http.ts Outdated
@build-analysisbuild-analysisBot mentioned this pull request Mar 6, 2025
Closed
@pavelsavara
pavelsavara marked this pull request as ready for review March 7, 2025 11:56
@pavelsavara
pavelsavara requested a review from lewing as a code ownerMarch 7, 2025 11:56
akoeplinger
akoeplinger approved these changes Mar 7, 2025
View reviewed changes
Comment threadsrc/libraries/Common/tests/System/Net/Prerequisites/NetCoreServer/Handlers/EchoHandler.cs Outdated
Comment threadsrc/libraries/Common/tests/System/Net/Http/ResponseStreamTest.cs
@pavelsavara@akoeplinger
Update src/libraries/Common/tests/System/Net/Prerequisites/NetCoreSer…
31d4106
…ver/Handlers/EchoHandler.cs
Co-authored-by: Alexander Köplinger <alex.koeplinger@outlook.com>
campersau
campersau approved these changes Mar 7, 2025
View reviewed changes

@campersaucampersau left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Some nits

Comment threadsrc/libraries/Common/tests/System/Net/Http/ResponseStreamTest.cs Outdated
Comment threadsrc/libraries/Common/tests/System/Net/Http/ResponseStreamTest.cs Outdated
Comment threadsrc/libraries/Common/tests/System/Net/Prerequisites/NetCoreServer/Handlers/EchoHandler.cs Outdated
pavelsavaraand others added 4 commits March 7, 2025 14:03
@pavelsavara@campersau
Update src/libraries/Common/tests/System/Net/Http/ResponseStreamTest.cs
2e030b6
Co-authored-by: campersau <buchholz.bastian@googlemail.com>
@pavelsavara@campersau
Update src/libraries/Common/tests/System/Net/Prerequisites/NetCoreSer…
a7559fd
…ver/Handlers/EchoHandler.cs
Co-authored-by: campersau <buchholz.bastian@googlemail.com>
@pavelsavara@campersau
Update src/libraries/Common/tests/System/Net/Http/ResponseStreamTest.cs
0c2c364
Co-authored-by: campersau <buchholz.bastian@googlemail.com>
@pavelsavara
Merge branch 'main' into browser_http_mute_abort
f2f2580
@pavelsavara

pavelsavara commented Mar 7, 2025

Copy link
Copy Markdown
MemberAuthor

/ba-g known CI issue #110805

@pavelsavara
pavelsavara merged commit 3161a29 into dotnet:mainMar 7, 2025
@pavelsavara
pavelsavara deleted the browser_http_mute_abort branch March 7, 2025 16:51
@pavelsavara

pavelsavara commented Mar 7, 2025

Copy link
Copy Markdown
MemberAuthor

/backport to release/9.0

@github-actions

github-actionsBot commented Mar 7, 2025

Copy link
Copy Markdown
Contributor

Started backporting to release/9.0: https://github.com/dotnet/runtime/actions/runs/13725376350

@github-actionsgithub-actionsBot mentioned this pull request Mar 7, 2025
Merged
4 tasks
pavelsavara added a commit to pavelsavara/runtime that referenced this pull request Mar 7, 2025
@pavelsavara
backport ofdotnet#113014with modifications for Net8
a139517
@pavelsavarapavelsavara mentioned this pull request Mar 7, 2025
Merged
4 tasks
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Apr 7, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@akoeplingerakoeplingerakoeplinger approved these changes
@javiercnjaviercnjaviercn approved these changes
@marafmarafAwaiting requested review from maraf
@lewinglewingAwaiting requested review from lewinglewing is a code owner
+1 more reviewer
@campersaucampersaucampersau approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@pavelsavarapavelsavara

Labels

arch-wasmWebAssembly architecturearea-System.Net.Httpos-browserBrowser variant of arch-wasm

Projects

None yet

Milestone

10.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@pavelsavara@akoeplinger@campersau@javiercn@lewing