Use threading.Event for _eventloop_set instead of anyio.Event
#1293
Conversation
davidbrochart
force-pushed
the
fix_eventloop_set
branch
from
4fec5b7 to
eba6c5c
Compare
|
Thanks! |
|
You will have to test this manually as much of the event loop code isn't tested in CI. There is a screenshot of a simple workflow in #1265. |
|
@ianthomas23 thanks for the pointer! When I run the test from #1265: I get a hang, both before and after this PR. I think this comment means that it not working is a known issue, is that right? Do we have an open Issue to track fixing |
|
I can reproduce the issue. |
It was working fine for the single-shot use case that nearly all end users want of setting the event loop either at the start or once later via e.g. For the more complicated use case of changing the event loop, yes that is a known issue but not recorded as a separate github issue as I was waiting for clarification with a reproducer, and lacking that I have not looked at it further.
Sure. Testing is a problem. We can add a test to catch the current failure mode as that is pretty fundamental. But testing that both ipykernel and matplotlib are responsive after a plot window is shown is complicated as we'd have to grab an external window on headless CI, etc, and this is probably not something that anyone wants in the ipykernel repo. I have been thinking about how we test the grey area between matplotlib and ipython/ipykernel/jupyter longer term and I have some ideas, but it is not something we can quickly add now. |
|
Yeah, directly testing that the plot window is responsive is hard. But we should be able to test:
I think timers won't fire in the cases where the window is unresponsive. That should get the basics that neither event loop is totally blocking the other which is the usual failure mode when we have a regression here, even if it isn't total coverage. |
| # Stop the async task that is waiting for the eventloop to be set. | ||
| self._eventloop_set.set() | ||
| # Stop the async task that is waiting for the eventloop to be set. | ||
| self._eventloop_set.set() |
There was a problem hiding this comment.
There was a problem hiding this comment.
or if that's overkill you could use:
def current_token() -> tuple[Literal["asyncio"], AbstractEventLoop] | tuple[Literal["trio"], trio.lowlevel.TrioToken]:
lib = sniffio.current_async_library()
if lib == "asyncio":
return lib, asyncio.get_running_loop()
if lib == "trio:
return lib, trio.lowlevel.current_trio_token()
def from_thread_run_sync[**P](token: tuple[Literal["asyncio"], AbstractEventLoop] | tuple[Literal["trio"], trio.lowlevel.TrioToken], fn: Callable[P, object], /, *args: P.args, **kwargs: P.kwargs) -> None:
lib, tok = token
if lib == "asyncio":
tok.call_soon_threadsafe(functools.partial(fn, *args, **kwargs)
return
if lib == "trio":
trio.from_thread.run_sync(functools.partial(fn, *args, **kwargs), trio_token=tok) async def _wait_to_enter_eventloop(self):
self.kernel._token = current_token()
await self.kernel._eventloop_set.wait()
await self.kernel.enter_eventloop()def stop(self):
from_thread_run_sync(self._token, self._eventloop_set.set)
Fixes #1292.