jupyter / jupyter_client

Jupyter protocol client APIs
https://jupyter-client.readthedocs.io
BSD 3-Clause "New" or "Revised" License
383 stars 283 forks source link

Use tornado 6.2's PeriodicCallback in restarter #822

Closed vidartf closed 2 years ago

vidartf commented 2 years ago

Since tornado 6.2, the PeriodicCallback will now await any callbacks that are awaitable (https://github.com/tornadoweb/tornado/pull/2924). So we no longer need to wrap the async restarter's poll method in a run_sync call. This allows us to not depend on nest_asyncio with the default configuration of jupyter client, so we can avoid some of its pitfalls (e.g. https://github.com/jupyterlab/jupyterlab/issues/11934).

vidartf commented 2 years ago

Note that this by itself will likely not fix the issue in https://github.com/jupyterlab/jupyterlab/issues/11934 for all users. Anyone who uses any of our sync classes might still be experiencing issues. Or it could be that the issue only appears for PeriodicCallback, I would have to dig a little deeper into the logic.

To clarify that issue: If we call nest_asyncio.patch() while we are already inside a asyncio.base_events._run_once() call, and the instance has more items in its queue still left to process, then there is a risk (race?) that the run_until_complete inside run_sync modifies the queue of the event loop. That will cause an unrecoverable error in asyncio ("can't pop from deque").

vidartf commented 2 years ago

~I see that the xeus kernel test in the downstream check fails on a run_sync call. Maybe it is same root cause of that issue?~ EDIT: I see this is failing on master as well, and seems unrelated

blink1073 commented 2 years ago

Let's see if we can get all green without xeus-cling.

vidartf commented 2 years ago

Is it okay to release this as a patch release when it bumps the tornado dependency version?

blink1073 commented 2 years ago

Yeah, let's try that, releasing now.