Fix asyncio error when opening notebooks

[dleen]

This is a fix for jupyter/notebook#6164

We are only applying nest_asyncio when run_sync has been called
which means there could be tasks queued that are unpatched e.g. from
the original issue:

<Task pending coro=<HTTP1ServerConnection._server_request_loop() running at /apps/python3/lib/python3.7/site-packages/tornado/http1connection.py:823>

which originates from Tornado.

A similar issue was reported in nest-asyncio: erdewit/nest_asyncio#22
where the solution is to call apply on import so that unpatched tasks
do not get created.

[davidbrochart]

The problem I see is that we still want to use nest-asyncio only when absolutely needed, because it patches the event loop (it cannot work with e.g. uvloop, and can potentially have side effects...).

[davidbrochart]

Not needed anymore?

[kevin-bates]

I'm guessing this needs to be moved into the exception handling so the new loop is patched (assuming the global apply is retained).

[dleen]

Yeah it's still needed, but it could have probably gone inside the exception handling

[kevin-bates]

I find this issue has a resemblance to the Windows event loop policy issue that existed for a couple of tornado releases. In #686 @minrk makes a point that this kind of (my words) "global patch" mechanism should be applied at the application layer and not as a side-effect of an import - which makes sense.

[davidbrochart]

Right, but because it is a patch, we wish we didn't have to use it, and we try to use it only when necessary. The proper solution is to not have to use run_sync, and use the async API.

[kevin-bates]

I understand and agree, although the "proper solution is to not have to use run_sync, and use the async API" is easier said than done. For example, PR #717 is introducing this code in order to induce synchronous behavior.

At any rate, it sounds like this issue (assuming I'm understanding things) should be applied at the application level and not here.

[dleen]

I agree that this should be applied at the application level. Let me look into how that might be done.

Is it fair to say that notebook is the only application that relies on this functionality? Maybe we should consider removing nest_asyncio completely from jupyter_client since without a top level patch every application that needs this functionality would need to apply it.

[davidbrochart]

Is it fair to say that notebook is the only application that relies on this functionality?

I don't think so, all applications that don't use the async API need this functionality.

[dleen]

Closing in favor of making the fix in the application instead of the client: jupyter/notebook#6221

[qci-amos]

I don't understand why, but we're seeing this nest_asyncio being applied:

jupyter_client/jupyter_client/utils.py

Lines 13 to 20 in 490298e

	try:
	loop = asyncio.get_event_loop()
	except RuntimeError:
	loop = asyncio.new_event_loop()
	asyncio.set_event_loop(loop)
	import nest_asyncio # type: ignore
	nest_asyncio.apply(loop)

and we're hitting an exception matching erdewit/nest_asyncio#22

is it not dangerous for this code in jupyter_client to patch a loop if:

• it's a pre-existing loop which
• has any prior tasks which
• were created before any nest_asyncio was applied?

[dleen]

Where are you seeing the issue? You should try a similar approach to: jupyter/notebook#6221 @qci-amos

And I agree with your points. That's why we had to change approach to patch the loop in the application (in the case in my PR it was notebooks). If we could patch all clients then this code could be removed.

[qci-amos]

Unfortunately, I don't have a lot of background to offer right now -- I'm just trying to help others debug their code. I don't understand the context in which this problem arose let alone have a reproducible case.

All I can say about my case is they are making an attempt to not use nest_asyncio because it's under suspicion (don't know details of that either, but I'm not convinced their suspicion is merited). But that's a question for you too -- is it possible in real applications / practical to use Jupyter w/o nest_asyncio?

If we could patch all clients then this code could be removed.

You don't consider this to be an option?

import nest_asyncio
nest_asyncio.apply()

[dleen]

Just to be clear: by initializing nest_asyncio in notebook it has resolved the issue this PR originally set out to solve. So if you want to try you can upgrade notebook==6.4.6 and get the latest jupyter_client and see if you still have the issue.

I was able to reproduce this issue easily enough using notebook==6.4.4 and any jupyter_client > 7. It requires you to be quick, but if you launch the notebook server, create a new notebook from the tree view, wait for the new tab to open, and as quickly as possible execute some code e.g. 1 + 1 then in the notebook server log you should see an Exception being thrown for the event loop.

If you aren't using notebook then you can investigate adding:

import nest_asyncio
nest_asyncio.apply()

to your application code to initialize it as early as possible: before launching any tasks.

Even better would be migrating your application to only use the asynchronous methods of jupyter_client and not relying on the sync ones.

But that's a question for you too -- is it possible in real applications / practical to use Jupyter w/o nest_asyncio?

I'm not an expert on this either, but my understanding is that this nest_asyncio stuff is only needed to maintain compatibility with the older sync code. If you can rely only on the async ones then in theory you won't ever hit the code that needs nested loops.

You don't consider this to be an option?

It would require a long deprecation cycle I believe:

• Identify all applications/consumers of jupyter_client
• For each consumer: either introduce nest_asyncio or remove the sync dependencies
• Release a new version of each
• Wait for users to move onto the new version
• Remove the code from jupyer_client

[qci-amos]

Ok, thank you for your comments @dleen!