-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
[wasm][mt] throw from blocking wait on JS interop threads #97052
[wasm][mt] throw from blocking wait on JS interop threads #97052
Conversation
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
src/libraries/System.Private.CoreLib/ref/System.Private.CoreLib.ExtraApis.cs
Outdated
Show resolved
Hide resolved
The current approach doesn't solve the blocking wait in JS interop, which is then throwing the exception for us too.
We discussed it in chat and agreed on the introduction of a new flag to signal that the wait comes from JS interop code. The flag can be added to the Monitor class and thus simplify the thread JS interop thread detection. |
…row-from-blocking-wait
To not mix intree references and source project
This replaces the previous context base and makes it possible to disable throw for blocking calls in JS interop
...s/System.Private.CoreLib/src/System/Runtime/InteropServices/JavaScript/JSProxyContextBase.cs
Outdated
Show resolved
Hide resolved
...rvices.JavaScript/src/System/Runtime/InteropServices/JavaScript/Interop/JavaScriptExports.cs
Outdated
Show resolved
Hide resolved
...rvices.JavaScript/src/System/Runtime/InteropServices/JavaScript/Interop/JavaScriptExports.cs
Show resolved
Hide resolved
New fail to look into:
|
It's unrelated to your PR, please add it to #96628 |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run runtime-wasm |
Azure Pipelines successfully started running 1 pipeline(s). |
This is unrelated. |
Quick list of APIs, for which we need to validate that they internally use something we already addressed so far.
I'm sure I missed some places. |
Lets get this in and I can extend the tests and API coverage in follow up PR. |
@@ -77,6 +82,12 @@ public static bool IsEntered(object obj) | |||
public static bool Wait(object obj, int millisecondsTimeout) | |||
{ | |||
ArgumentNullException.ThrowIfNull(obj); | |||
#if FEATURE_WASM_THREADS |
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.
Let's make helper method for this and localize the message.
The method should be "public" but suppressed same like ThrowOnBlockingWaitOnJSInteropThread
.
So that we could use it in other places in the runtime.
signal.Wait(); | ||
Monitor.ThrowOnBlockingWaitOnJSInteropThread = threadFlag; |
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.
should this be in try-finally
?
could .Wait ever throw ?
@radekdoulik
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.
I'll will change that on my PR #97832
The project files have now diferent dependencies
* Update solution files after changes in #97052 The project files have now diferent dependencies * Update System.Runtime.InteropServices.JavaScript.sln * Update System.Net.Http.sln * Update System.Net.WebSockets.Client.sln --------- Co-authored-by: Viktor Hofer <[email protected]>
Also add test
Contributes to #76958