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][mt] Thread affinity for HTTP and WS #85244

Closed
wants to merge 2 commits into from

Conversation

pavelsavara
Copy link
Member

@pavelsavara pavelsavara commented Apr 24, 2023

Keep HTTP and WS on the same thread where it was initiated in the browser.
Because the JavaScript objects and the network sessions are bound to particular web worker.

Luckily the OperatingSystem.IsBrowser() is a constant and so it should be eliminated by IL linker or JIT.

@pavelsavara pavelsavara added arch-wasm WebAssembly architecture area-System.Net.Http labels Apr 24, 2023
@pavelsavara pavelsavara added this to the 8.0.0 milestone Apr 24, 2023
@pavelsavara pavelsavara self-assigned this Apr 24, 2023
@ghost
Copy link

ghost commented Apr 24, 2023

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

Issue Details

Keep HTTP and WS on the same thread where it was initiated in the browser.
Because the JavaScript objects and the network sessions are bound to particular web worker.

Author: pavelsavara
Assignees: pavelsavara
Labels:

arch-wasm, area-System.Net.Http

Milestone: 8.0.0

@campersau
Copy link
Contributor

Do these HttpContent implementations also need to be updated?
https://github.com/dotnet/runtime/tree/main/src/libraries/System.Net.Http.Json/src/System/Net/Http/Json

@pavelsavara
Copy link
Member Author

Do these HttpContent implementations also need to be updated? https://github.com/dotnet/runtime/tree/main/src/libraries/System.Net.Http.Json/src/System/Net/Http/Json

Good point. It spreads to all corners actually.
Anything that could call and await HTTP or WS need to have it.
Anything that awaits Stream needs to have it too, as the stream could be HTTP stream.

Even our unit tests have ConfigureAwait(false) for some reason.

Is there better way ?

@lambdageek
Copy link
Member

@pavelsavara I tried this in #85109 and it's enough to get the Blazor Weather Report page to fetch its data correctly.

But we probably need to solve this in another way. Changing all the ConfigureAwait(false) everywhere isn't going to be sustainable. And it will miss other mechanisms that push work off to other threads.

We will probably need to do something to queue the JS invoke on the correct thread under the covers.

@ghost ghost locked as resolved and limited conversation to collaborators May 24, 2023
@pavelsavara pavelsavara deleted the browser_mt_await branch September 2, 2024 15:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-System.Net.Http
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants