-
Notifications
You must be signed in to change notification settings - Fork 21
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
Async #29
base: next
Are you sure you want to change the base?
Async #29
Conversation
add ability for host functions to suspend vm and yield values to host, and then to resume, potentially receiving value from host
on a side note: the hell am i seeing, did this just worked on the first try? what black magic is this? i guess i haven't tested the indirect call... hmm, should i deduplicate that code?
I found what i broke, (and fixed it) yay! On that note, it'd be cool to run entire test suite but interrupt it randomly (which can be done with a should-suspend user callback that uses a random number generator) |
i did, and it failed the Also added my async-testing edits to test runner, but under a "test_async" feature for now. Not sure how to do it properly - test both normal running and periodically suspended one, and whether it's even desirable |
i made the api very opt-in: the whole thing is planned to be behind feature flag, and even if some other dependency adds that feature - to engage with "suspend" functionality user needs to go out of their way and call "_coro" versions of functions - the "old" ones just return error when execution is suspended, keeping things backwards-compatible. However, does it make sense to do all that? It adds burden on users who do want to use async/suspend to remember to call "_coro" versions. and how often do you run code in a sandboxed environment, but aren't interested in controlling how much execution time it takes? Maybe if you're running trusted code, or keep wasm executors in separate processes that you can just kill if they are taking too long. Should i just modify existing api to return "maybe result maybe suspended execution" (rather than "_coro" versions), and not bother with feature flag? |
something that could be useful, but beyond the scope of this pr |
…and qol functions
So just as i initially planned, resume/async functionality if very opt-in: user needs to enable "async" feature, and even if it's enabled by something else in the dependency tree, none of user's code should break (unless they match on error, where i followed existing practice and feature-gated relevant enum variant). They also need to go out of their way and set set_suspend_conditions on host or create coroutine functions, and use "*_coro" functions when calling exported function or instantiating modules |
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.
Looks really cool so far! I'm not fully on board with the API / amount of internal changes, and don't have a lot of time at least in January so it might take some time for me to fully review this and play around with some ideas on how to simplify it. The general principals definitely looks like something I want to add though.
That's fair enough There are some compromises here I'm not entirely happy with either. If you or prefer different variants that ones i took, or have entirely different ideas, i'd rewrite with your directions in mind I'd prefer if CoroState were consumed self by the "resume" and on suspension returned self in "Suspended", variant, but I don't see a way to make it dyn-compatible. I'm guessing that's why it doesn't consume self in experimental std trait either
(I could also not have CoroState at all, and make "host coro"s store their intermediary state in store's userdata (and add said userdata). Also could do away with Another compromise is the degree of op-in-ness of api vs niceness of the code. I am being cautious not to introduce anything to public api if there is no feature, and "conditional pub" approach i use here makes code a fair bit less nice. I could try to make a macro that makes thing conditionally public, or just let async-specific types (and maybe even functions) spill into namespace even if there is no feature enabled, and only conditionally disable the code that affects runtime performance and lets users create host coroutines or set suspend conditions |
instead of putting " |
Related to issue: #17
This is not final, but I'd like to present my prototype of async/resume support and get feedback
It adds facilities for suspending and resuming execution of wasm code.
Namely:
from what I can see, I need to do at least few more things:
add typed wrappers for async function types and result enums (so that "yielded value" and "resume argument" are typed),for comparable to "typed functions" result would require a "coroutine types registry" in store that would check types when exporting. not impossible, but would make this pr too bigwhen you call wasm function, you may get anything any imported function yielded as yielded value, not sure how would ensuring type safery of that would workconsider adding lifetime to "yielded value" and "resume argument" (so that "resume argument" could reference something from host caller and "yielded value" could reference something in "suspended host coro"), tests in rustnope, very likely undoable withoutunsafe
and very careful lifetimancy