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

[WASI] WasiEventLoop to keep Task alive #106633

Merged
merged 11 commits into from
Aug 28, 2024

Conversation

pavelsavara
Copy link
Member

@pavelsavara pavelsavara commented Aug 19, 2024

  • WasiEventLoop to not use WeakReference - when the task is collected the loop could be infinite
  • add GC.Collect() to HTTP test to test it works now
  • test pollable clock too via Task.Delay()
  • bump wit-bindgen-cli to 0.30, also add powershell scripts
  • create JCO sample
  • RegisterWasiPollable with CancellationToken argument

@pavelsavara pavelsavara added arch-wasm WebAssembly architecture area-Interop-mono os-wasi Related to WASI variant of arch-wasm labels Aug 19, 2024
@pavelsavara pavelsavara added this to the 10.0.0 milestone Aug 19, 2024
@pavelsavara pavelsavara self-assigned this Aug 19, 2024
Copy link
Contributor

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

- Pollable lifetime by lifetime of the Task
@pavelsavara pavelsavara changed the title [WASI] improve pollable [WASI] WasiEventLoop to keep Task alive Aug 26, 2024
@pavelsavara pavelsavara marked this pull request as ready for review August 26, 2024 18:02
@pavelsavara
Copy link
Member Author

@dicej please review


namespace System.Threading
{
internal static class WasiEventLoop
{
private static List<WeakReference<TaskCompletionSource>> s_pollables = new();
// TODO: if the Pollable never resolves and and the Task is abandoned
Copy link
Contributor

@dicej dicej Aug 26, 2024

Choose a reason for hiding this comment

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

Here's my naive attempt at this. I'm sure yours is better, but one thing to make sure of is that we give all tasks a chance to run once any of them have been cancelled (see this comment). I can't tell if your code accounts for this.

FWIW, this is the code I've been using to test HttpClient timeouts. It's paired with this server 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.

My solution will cancel the Task synchronously when the CancellationTokenSource.Cancel is called because of the CancellationTokenRegistration callback.
And that could possibly trigger execution of other synchronous/inline continuations.
It seems to me, that synchronous cancellation is better. And it's enough to pass the test.

I added test similar to yours with some azure endpoint, which is not perfect, but I plan to fix it later.

@pavelsavara
Copy link
Member Author

@pavelsavara pavelsavara merged commit 0a16679 into dotnet:main Aug 28, 2024
146 of 156 checks passed
@pavelsavara pavelsavara deleted the wasi_gc_pollable branch August 28, 2024 16:53
jtschuster pushed a commit to jtschuster/runtime that referenced this pull request Sep 17, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Sep 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-Interop-mono os-wasi Related to WASI variant of arch-wasm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants