-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
[release/8.0][browser][http] mute JS exceptions about network errors + HEAD verb #113271
base: release/8.0-staging
Are you sure you want to change the base?
[release/8.0][browser][http] mute JS exceptions about network errors + HEAD verb #113271
Conversation
Tagging subscribers to this area: @dotnet/ncl |
cc @campersau |
@pavelsavara Thank you! I'll let you know how it works in the next release. BTW, do you guys set tickets to closed when they are released? or when should I expect the next release to happen? |
Tests passed on chrome, log |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Overview
This PR backports fixes to address unintended exception propagation in the browser HTTP client and to handle HEAD verb responses correctly in Firefox within Net8. Key changes include adding new tests for multiple HTTP methods including abort scenarios, updating the EchoHandler to handle delays and aborts appropriately, and modifying the wasm HTTP request handling to mute specific unhandled rejections.
Reviewed Changes
File | Description |
---|---|
src/libraries/Common/tests/System/Net/Http/ResponseStreamTest.cs | Added new test cases covering different HTTP methods and abort scenarios for streaming responses. |
src/libraries/Common/tests/System/Net/Prerequisites/NetCoreServer/Handlers/EchoHandler.cs | Updated the echo handler to incorporate delay, buffering control, and proper handling of the HEAD method and abort scenarios. |
src/mono/wasm/runtime/http.ts | Modified the HTTP handling logic to improve the muting of unhandled promise rejections by refactoring error handling in abort scenarios. |
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/libraries/Common/tests/System/Net/Prerequisites/NetCoreServer/Handlers/EchoHandler.cs:76
- [nitpick] Consider using a pre-defined constant (such as HttpMethods.Head) instead of the string literal "HEAD" to maintain consistency with other HTTP method checks.
if(context.Request.Method == "HEAD")
It should land in Net 10 preview3 nightly in next few days. It would be awesome if you can try to use it to validate the fix and report back. That would decrease risk for the Net8 patch. https://aka.ms/dotnet/10.0.1xx/daily/dotnet-sdk-win-x64.zip
I think this will go out in 8.0.15 in April. |
Backport of #113014 to release/8.0 with modifications for Net8
Customer Impact
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.in Firefox when the verb is HEAD, the body is null. We should not fail because of that.
Contributes to #112172
Fixes #111992
Regression
Testing
Automated tests.
Risk
Low
Both issues are only triggered when user enabled streaming responses. That's opt-in feature in Net8.