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] Make HTTP and WS clients work #87567

Merged
merged 6 commits into from
Jun 23, 2023

Conversation

pavelsavara
Copy link
Member

@pavelsavara pavelsavara commented Jun 14, 2023

BrowserWebSocket

  • _thisLock for locking the state for MT access
  • use FEATURE_WASM_THREADS to conditionaly wrap the calls with SynchronizationContext.Send for all calls to JS
  • split ConnectAsyncCore and CreateCore to separate methods. CreateCore is synchronous and creates the JS proxy. The other one needs to be executed separately, so that Abort() before await ConnectAsync() works as expected. Yielding to main loop.

BrowserHttpHandler

  • WasmFetchResponse.ThisLock for locking the state for MT access
  • use FEATURE_WASM_THREADS to conditionaly wrap the calls with SynchronizationContext.Send for all calls to JS

Other

  • make JSSynchronizationContext possible to install on multiple threads
  • private draft of SynchronizationContextExtension hidden from API by CompatibilitySuppressions.xml
    • these are sync and async helpers for dispatching call to correct thread and propagating result and exception back
  • private draft of JSHost.CurrentOrMainJSSynchronizationContext
    • this allows to use main thread as fallback for JS interop, if there is no JSSynchronizationContext installed on current thread
  • private draft of JSObject.SynchronizationContext
    • this allows to dispatch call back to correct thread for the proxy
  • private draft of WebWorker hidden from API by CompatibilitySuppressions.xml
  • JSException from another thread doesn't have thread affinity and stack trace.
  • more thread affinity asserts

Contributes to #85592

@ghost
Copy link

ghost commented Jun 14, 2023

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

Issue Details

Ideas:

  • both HTTP and WS clients support only async APIs for long operations, including via Streams.
  • async operations are easy to schedule onto thread with JSSynchronizationContext installed
  • when you create HTTP or WS client on thread pool, your call would be dispatched to main thread instead.
  • the client would remember it's origin and dispatch subsequent calls to the correct thread/worker

Other

  • added SendAsync SendAsync<T> and Post<T> helpers which dispatching to SynchronizationContext and waiting for result.
Author: pavelsavara
Assignees: pavelsavara
Labels:

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

Milestone: 8.0.0

@andrijana44

This comment was marked as spam.

Copy link
Member

@lambdageek lambdageek left a comment

Choose a reason for hiding this comment

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

The only thing I'd be careful about is assuming that the only synchronization contexts that can be installed on a thread is either nothing or JSSynchronizationContext.

Also I'm not sure there's any good way to avoid a ton of allocation with the SynchronizationContextExtension methods, but using static lambdas that don't capture local variables or fields of this and instead passing extra arguments can decrease some of the allocations. (of course the call to the actual SynchronizationContext.Post(SendOrPostCallback callback, object? state) method will still need to allocate something - either a state object or a closure. but maybe some of the other allocations can be avoided by carefully constructing the code)

@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara pavelsavara force-pushed the browser_dispatch_to_web_worker branch from 58a07bf to 5799250 Compare June 22, 2023 11:39
@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@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 June 22, 2023 20:17
Comment on lines +135 to +150
#if FEATURE_WASM_THREADS
if (!abortController.IsDisposed)
{
abortController.SynchronizationContext.Send(static (JSObject _abortController) =>
{
BrowserHttpInterop.AbortRequest(_abortController);
_abortController.Dispose();
}, abortController);
}
#else
if (!abortController.IsDisposed)
{
BrowserHttpInterop.AbortRequest(abortController);
abortController.Dispose();
}
abortController.Dispose();
#endif
Copy link
Member

Choose a reason for hiding this comment

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

This should be valid

Suggested change
#if FEATURE_WASM_THREADS
if (!abortController.IsDisposed)
{
abortController.SynchronizationContext.Send(static (JSObject _abortController) =>
{
BrowserHttpInterop.AbortRequest(_abortController);
_abortController.Dispose();
}, abortController);
}
#else
if (!abortController.IsDisposed)
{
BrowserHttpInterop.AbortRequest(abortController);
abortController.Dispose();
}
abortController.Dispose();
#endif
if (!abortController.IsDisposed)
{
#if FEATURE_WASM_THREADS
abortController.SynchronizationContext.Send(static (JSObject abortController) =>
{
#else
BrowserHttpInterop.AbortRequest(abortController);
abortController.Dispose();
#if FEATURE_WASM_THREADS
}, abortController);
#endif
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Are you saying that I should shadow abortController name in the callback, so that I could have more DRY code ?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can further tweak it in follow-up PRs. I will merge it so that it has time to flow to Blazor over the weekend.

Copy link
Member

@lambdageek lambdageek left a comment

Choose a reason for hiding this comment

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

On the surface LGTM. I'm not super-familiar with the the implementations of these streams, though.

@pavelsavara pavelsavara merged commit 574ad01 into dotnet:main Jun 23, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jul 23, 2023
@pavelsavara pavelsavara deleted the browser_dispatch_to_web_worker branch September 2, 2024 15:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants