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

Stack switching: make pystate properly save and restore asyncio state #4532

Merged
merged 6 commits into from Feb 23, 2024

Conversation

hoodmane
Copy link
Member

@hoodmane hoodmane commented Feb 18, 2024

This updates pystate.c to save and restore the current event loop task. This allows us to stack switch out of one async task and then block for the completion of another async task.

We also add callPyObjectMaybeSuspending which uses promisingApply if stack switching is available and otherwise makes a normal call. We add a private keyword argument _may_syncify to create_once_callable and have the event loop use this so async tasks can stack switch. We also make the promise handles use callPyObjectMaybeSuspending so that promise.then, promise.finally_, etc can use stack switching.

This creates the funny situation described in #4401 where a handler can stack switch only if the entrypoint was an async function. I think I can live with this.

Example

from js import document, sleep
from pyodide.ffi import create_proxy

def f(e, entry="f"):
   print(f"Does this work? {entry}")
   try:
      sleep(100).syncify()
   except:
      print(f"no {entry}")  
   else:
      print(f"yes {entry}")  

async def g(e):
  f(e, entry="g")

document.addEventListener("click", create_proxy(f))
document.addEventListener("click", create_proxy(g))

prints:

Does this work? f
no f
Does this work? g
yes g

A few API considerations:

  1. Should we eliminate pyodide.runPythonSyncifying? After this PR it's sufficient to use pyodide.runPythonAsync.
  2. Should we eliminate PyProxy.callSyncifying? It suffices to make it an async function. If you have a non async function you can make an async wrapper around it (see example).
  3. Should we eliminate syncify and just tell people to use asyncio.run_until_complete instead? syncify has problems like Using asyncio.gather() with syncify() raises error #4398 which seem hard to fix.

cc @rth @ryanking13 @kor0p @dom96 @garrettgu10

Checklist

  • Add a CHANGELOG entry
  • Add / update tests
  • Add new / update outdated documentation

@hoodmane
Copy link
Member Author

One problem with asyncio.run_until_complete is that it already does not block when JSPI is not available, so it can't raise an exception if blocking is impossible. We should probably add a new API for "block or raise an exception if you can't". But it probably should take the Future/Promise as an argument since adding a method to Futures and Promises, though tempting, has some limitations e.g., #4398. Though if we do want to keep a method, we could patch the base Future rather than making a subclass.

Another TODO is to add a good way to query whether or not syncify() can work. This involves annoying bookkeeping since currently JSPI provides no way to ask the runtime (WebAssembly/js-promise-integration#29). I think we can add appropriate bookkeeping in our EM_JS wrappers.

@ryanking13
Copy link
Member

Thanks for keep improving JSPI related stuff!

Should we eliminate syncify and just tell people to use asyncio.run_until_complete instead? syncify has problems like #4398 which seem hard to fix.

Yes, I am +1 for using asyncio.run_until_complete if possible, so we can abstract JSPI if possible.

@hoodmane
Copy link
Member Author

What about the fact that it might just do nothing if jspi is not available? Surely we should offer an alternative API that raises if it can't block? Or should we make run_until_complete do that?

@ryanking13
Copy link
Member

I think asyncio.run_until_complete should do that (raising an error, or at least showing an warning), which will make existing codebases more portable.

@hoodmane hoodmane merged commit 84977b8 into pyodide:main Feb 23, 2024
33 of 38 checks passed
@hoodmane hoodmane deleted the syncify-and-async branch February 23, 2024 00:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants