-
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
[WASI] timers based on wasi:clocks #105879
Conversation
src/mono/System.Private.CoreLib/src/ILLink/ILLink.Descriptors.WASI.xml
Outdated
Show resolved
Hide resolved
src/mono/System.Private.CoreLib/src/ILLink/ILLink.Descriptors.WASI.xml
Outdated
Show resolved
Hide resolved
src/mono/System.Private.CoreLib/src/ILLink/ILLink.Descriptors.WASI.xml
Outdated
Show resolved
Hide resolved
- use ILLinkDescriptorsLibraryBuildXml instaed of ILLinkDescriptorsXmls
src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems
Outdated
Show resolved
Hide resolved
…b.Shared.projitems Co-authored-by: Marek Fišera <mara@neptuo.com>
{ | ||
WasiEventLoop.DispatchWasiEventLoop(); | ||
while (!mainTask.IsCompleted) |
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.
This is probably too naive solution.
There could be other pending tasks scheduled to C# thread pool.
But not in the await chain leading to this entrypoint.
Such code/tasks/continuations could have (expected) externally observable side effects (after we exit the entry point).
But I don't know how to do that at this point. So I guess it will be some next PR.
cc @dicej
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.
In the browser we schedule browser callback any time the new job triggers ThreadPool.RequestWorkerThread()
, but we could not do it here.
Maybe we need to run ThreadPoolWorkQueue.Dispatch();
until there was no call to ThreadPool.RequestWorkerThread()
before we return from the entry point.
cc @SingleAccretion , any thoughts ?
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 agree that full event loop emulation requires what is effectively:
bool workItemsLeft;
do
{
workItemsLeft = ThreadPoolWorkQueue.Dispatch();
}
while (workItemsLeft)
I think it's ok to wait for the top-level task only here. It matches "normal" platform's behavior:
ThreadPool.QueueUserWorkItem(x => { Thread.Sleep(100); Console.WriteLine(); });
> dotnet run
; Nothing printed because thread pool threads are background threads.
So it seems unlikely our tests would depend on it.
It does not match Browser's behavior, of course. It is a point to debate whether we'd want the eventual async main experience to match browser or not-browser.
/cc @sbomer for the library descriptor changes as an FYI mostly. |
thanks @SingleAccretion
I think parity with desktop/server dotnet is more interesting than parity with browser dotnet. I also tested following on desktop dotnet and it doesn't print either. // no await
Task.Delay(10000).ContinueWith(_=>{
Console.WriteLine("Later");
}); In the browser, the dotnet/mono application keeps running, because we execute the main entrypoint as any other export. Nodejs (as another example of single threaded engine with promises) is not exiting the process until promises settle. In library mode, if the call to such UCO/export scheduled un-awaited tasks/jobs/continuations, they will execute on next call to any export/UCO. I wonder what problems that will cause. For this PR, I think we could keep it simple (as is for now) and we will see later if there is any need to change this before or after WASIp3. |
Another interesting place would be the WASIp2 in library mode. We will need to add same or similar top level event loop to each export/UCO (so that HTTP client would work). We can do that by calling this new But even if/after that becomes public C# API, I don't like that UCO pattern. I would prefer this to be part of the UCO wrapper code in C together with the initialization of the runtime. I'll probably try to prototype it. |
I agree it is a problematic pattern w.r.t. versioning. My hope is that the eventual P3 with host-side event loop will allow us to get rid of it. I view spinning the event loop as equivalent to Task marshalling in JS interop. As with task marhshalling, without it, you can't run anything async. The major difference with JS interop is that the generator is not part of the runtime, and there is no public API that it can use to spin the loop, so we're adding a "backdoor" for it. I suspect that if we had a "spin the thread pool" API in .NET FW 1.0 BCL, we wouldn't be that upset about using it in the generator.
I will note that adding any marshalling to UCOs is a non-starter in RyuJit because there is no wrapper - UCO inlines the RPI transitions. And, more generally, it would go against the design of UCO. |
TimerQueue
on top ofwasi:clocks
WasmTestRunner
for wasi now calls the event loopPoll
WasiHttpHandler
Pollable
/Task
registration inWasiEventLoop
ILLinkDescriptorsLibraryBuildXml
and use it to protectWasiEventLoop
members. Related issueFixes #105810
Contributes to #102894
cc @dicej