Fix shutdown and cleanup behavior

[blink1073]

Fixes #771

$ jupyter qtconsole
qt.qpa.drawing: Layer-backing can not be explicitly controlled on 10.14 when built against the 10.14 SDK
$

Includes improvements made while working on jupyter-server/jupyter_server#792

Also avoids leaving event loops open at exit in a few more places.

[blink1073]

This is now working well in conjunction with jupyter-server/jupyter_server#792. Note that the downstream jupyter_server test passes with these changes, but that PR requires these changes in order to exit cleanly.

[blink1073]

This actually breaks nbclient, I can repro locally. Working a fix.

[blink1073]

Okay, this now works for all the test cases:

• qtconsole shutdown
• Centralize app cleanup jupyter-server/jupyter_server#792
• existing downstream tests

[kevin-bates]

These changes look good to me - thank you @blink1073!

I confirmed that the noise following the exit of jupyter qtconsole when 7.2.2 is used is now completely silent.

@ccordoba12 or @mrclary - are either of you able to take this for a spin as well? (Thanks for the great reproduction scenario!)

[mrclary]

@kevin-bates @blink1073, thanks for the quick updates!
I can confirm that this PR does not produce the ZMQ errors that I reported in #771.

The baseline:

>> conda create -n jup python=3.9.12 jupyter_client=7.2.2 qtconsole=5.3.0
>> conda activate jup
(jup) >> jupyter qtconsole

produces the errors that I reported on shutdown. Then using https://github.com/blink1073/jupyter_client/tree/fix-shutdown-behavior

(jup) >> pip install --no-deps ~/Documents/Python/jupyter_client
(jup) >> jupyter qtconsole

does not produce the errors on shutdown.

[blink1073]

Cool, let's give it a go.

[mariokostelac]

@blink1073 I might be wrong, but I think this causes nbclient leaking eventpoll FDs every time its util.just_run is called.

I've tried fixing it by stopping the loop in jupyter/nbclient#237, but

jupyter_client/jupyter_client/utils.py

Line 18 in 3697a0d

loop = asyncio.get_event_loop_policy().get_event_loop()

picks up a closed loop, and tries to run it, which fails.

I'm interested in why the method doesn't create a new loop every time there's no running loop?

        try:
            loop = asyncio.get_running_loop()
        except RuntimeError:
            # Workaround for bugs.python.org/issue39529.
            #try:
             #   loop = asyncio.get_event_loop_policy().get_event_loop()
            #except RuntimeError:
            loop = asyncio.new_event_loop()
            asyncio.set_event_loop(loop)

I'd be quite happy to fix the issue, but I think I need a bit more context to understand what's expected from run_async function.

[blink1073]

Hi @mariokostelac, I'd be concerned about another part of the application having a handle on a non-running loop. I think maybe we need to check if the loop returned by asyncio.get_event_loop_policy().get_event_loop() is closed before assigning a new one.

[mariokostelac]

Hi @mariokostelac, I'd be concerned about another part of the application having a handle on a non-running loop.

@blink1073 any chance you can explain this a bit more? Do you think that stopping and closing the loop in the linked PR is problematic? If we don't do it, we definitely leak open loops, and consequently, eventpoll FDs.

To be transparent upfront, I'm no expert on async subsytem in python, so take everything with a grain of salt.
My understanding is that the problem lies in

jupyter_client/jupyter_client/utils.py

Line 18 in 3697a0d

loop = asyncio.get_event_loop_policy().get_event_loop()

. If there is a running loop in the thread running a function,

jupyter_client/jupyter_client/utils.py

Line 14 in 3697a0d

loop = asyncio.get_running_loop()

will get it, and we'll enqueue our function at the end of it. That's a "happy path", it's all good.

If there's no running loop, we need some loop to run our coroutine. New loop seems perfectly reasonable to me, but we go further, and go straight to the event policy (to avoid the deprecation), and risk that we get a loop that's closed already.

Why do we do that? Is it to reduce loop creations? Is the problem that nbclient's just_run doesn't go that far to get a loop to run on?

Also, what's the expectation from "current loop"? Is the code I've suggested in nbclient lib breaking some contract by stopping and cancelling the loop?

[blink1073]

My concern is if another part of the program has called asyncio.get_event_loop_policy().get_event_loop() or doesn't have warnings as errors turned on, and used asyncio.get_event_loop(), but that loop has not yet been started, we would throw it away instead of running it. What I'm suggesting is something like:

        try:
            loop = asyncio.get_running_loop()
        except RuntimeError:
            try:
                loop = asyncio.get_event_loop_policy().get_event_loop()
                if loop.is_closed():
                     loop = asyncio.new_event_loop()
            except RuntimeError:
                loop = asyncio.new_event_loop()
            asyncio.set_event_loop(loop)

[mariokostelac]

Hmm, I get your concern. I wonder whether the solution I've suggested in nbclient is actually ok, or it leaves the system in some unexpected state. If latter is the case, we can change jupyter_client to get around it, but some other (unrelated) libraries could break.

Is it expected that get_event_loop always returns a non-closed loop?

[blink1073]

It isn't clear from the docs. In any case, the get_event_loop_policy().get_event_loop() was meant to be deprecated along with asyncio.get_event_loop(), but wasn't in 3.10 due to an oversight. We'll want to implement #755 for a proper fix.

[mariokostelac]

Thanks for the context. I've left more comments on the nbclient PR. From what I understand, it makes sense to make run_sync implementations between these two the same, and then migrate off get_event_loop at the same time.

On Fri 1 Jul 2022 at 16:14, Steven Silvester ***@***.***> wrote: It isn't clear from the docs <https://docs.python.org/3/library/asyncio-policy.html#asyncio.AbstractEventLoopPolicy.get_event_loop>. In any case, the get_event_loop_policy().get_event_loop() was meant to be deprecated along with asyncio.get_event_loop(), but wasn't in 3.10 due to an oversight. We'll want to implement #755 <#755> for a proper fix. — Reply to this email directly, view it on GitHub <#772 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAOUEC6NNSICXBJEDMPMHVTVR34LPANCNFSM5TTXJJOQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

-- .mk