Refactor Qtile LifeCycle

[DorianGray]

This PR refactors the process lifecycle related parts of Qtile to properly manage resource finalization and just generally cleans up the code.

Instead of running os.execv before python's builtin finalization
routines, this uses those same finalization routines to move os.execv to
the last thing done by the interpreter before termination.

[DorianGray]

CI failure seems to be unrelated :/

[ramnes]

yeah, let's run them again

[DorianGray]

ugh mypy, I'll need to get that working locally tomorrow

[DorianGray]

Added the appropriate mypy hints

[ramnes]

Why not just atexit.register(qtile.maybe_restart)?

[DorianGray]

maybe_restart is a bound method. I wanted to give all objects, especially qtile and the things it owns, a chance to be finalized before os.execv.

Edit: sorry, had a toddler climbing on me. The other big reason is that we want the atexit handler to be the last thing executed by the python vm since os.execv is going to instantly terminate without cleaning up. That means it needs to be the first thing registered. Beyond the need I felt that having process lifecycle management outside of qtile was a better design for testing, embedding in other programs, etc.

Also, the reason lifecycle is not a class is to reduce the number of objects that would still be alive atexit

[ramnes]

Okay, that's what I thought. :)

You made a perfect transition to what I was going to say next with your last sentence, eh: I'd prefer everything in lifecyle.py to be in a class so that we could avoid the setters and getters that are just proxies over global objects. I think that we don't care having such a class instance still alive at exit. I was thinking of something along these lines:

class Lifecyle:
    def __init__(self):
        self.behavior = Behavior.TERMINATE
        ...

    def teardown(self):
        if self.behavior = Behavior.RESTART:
            ...

lifecycle = Lifecycle()
atexit.register(lifecycle.teardown)

Then we could probably manage other things from Qtile's lifecycle... but I feel that's a bit redundant with SessionManager, so maybe we should just merge both features here?

PS: I do have toddlers yelling around as well, so I feel you!

[DorianGray]

Fair enough! I'll make that change in a bit.

[DorianGray]

Done!

[ramnes]

Cool, but did you see the last part of my previous comment? 👀

but I feel that's a bit redundant with SessionManager, so maybe we should just merge both features here?

[m-col]

I agree with the merge with SessionManager. It is managing the session after all! This may involve some playing around with conftest.py.

[DorianGray]

I'll see about merging lifecycle + session manager either tonight or tomorrow. I'll also go ahead and get rid of that setter... my reasons for keeping it were just to make the method signature match execv so it was kind of like a delayed call but that's probably not that valuable anyways.

[DorianGray]

Done!

[DorianGray]

segfault :/

[m-col]

Is it feasible to have the new session manager logic contained within the session manager itself, rather than having this bi-directional relationship between it and the qtile object?

[DorianGray]

qtile needs some way to signal the session manager that it would like to restart instead of terminate, what did you have in mind?

[m-col]

Relevant IRC logs:

14:30 < dditthardt> reworking stop/restart in qtile looks to be very invasive
14:31 < dditthardt> would involve moving all the event loop code out of qtile and into session manager
14:31 < dditthardt> but yeah, that does seem like the way it should eventually be
14:32 < dditthardt> it'd be super nice to have the qtile object care about how it's started/stopped
...
14:35 < dditthardt> termination signal handling and things
14:36 < dditthardt> is in qtile right now
14:36 < dditthardt> belongs in session_manager imo
14:36 < mcol> yeah, that sounds right
14:36 < dditthardt> then we won;t have to pass the event loop around anymore
14:37 < dditthardt> I'm kind of excited to work on that, the question is do that now or later?
14:37 < mcol> OK, looking at it I definitely agree with you
14:38 < mcol> it makes sense as a precursor to the atexit stuff
14:38 < dditthardt> the dbus/glib event loop binding code belongs...somewhere for now too
14:39 < mcol> or perhaps alongise the atexit stuff - could just put it in the same PR
14:39 < mcol> it doesn't look like it would be that much work, right?
14:39 < mcol> there aren't many use of self._eventloop in manager.py
14:40 < dditthardt> would you mind posting that reasoning on the github issue? I'll give it a shot later today
14:40 < mcol> sure thing
14:40 < dditthardt> ty
14:41 < ramnes> dditthardt: did you see svig comment in the code? :)
14:41 < ramnes> https://github.com/qtile/qtile/blob/master/libqtile/core/session_manager.py#L48
14:42 < ramnes> and we're 3.7+ now :)
14:42 < dditthardt> yeah
14:42 < ramnes> I'd love to see this
14:42 < ramnes> would make the architecture much better imo

[DorianGray]

planning on breaking out loop setup/teardown and atexit stuff into different files

[DorianGray]

segfault but tests pass! invasive work done, now to make it prettier

[DorianGray]

I should also mention that this slightly changes how different config hooks are fired.

For instance, in my config I was using asyncio.run as a crude method of having async handlers for hooks like startup and startup_once. With this change that no longer works, it errors informing the user that they cannot use asyncio.run inside of an existing event loop. Since my use case did not require awaiting a result, I simply changed asyncio.run to asyncio.create_task

[DorianGray]

Confirmed dbus is working normally after these changes. Was able to use both the notification widget as well as elparaguayo's upower/dbus based battery widgets successfully with no errors. I was able to send notifications and see them displayed in my bar with both notify-send out of process and libqtile.utils::send_notification.

[ramnes]

Thank you so much for your work, it looks like it really improves the overall code workflow! That being said, and maybe because I find this PR really important, the diff is still a bit too complicated to get my approval for two reasons:

1. I feel that the interaction between the three classes LifeCycle, QtileLoop and SessionManager complicates the overall architecture for a small benefit, since both the LifeCycle and QtileLoop classes are only used in SessionManager. What kind of advantage do you see in the current implementation? Could we merge LifeCycle into QtileLoop and include it in session_manager.py, for example?

2. Your small unnecessary modifications here and there really make it tedious to read the commits and will clutter the history if we merge them. I know it takes time on your side to change and I'm sorry I have to ask you to work on this since it makes me feel like I'm nitpicking, but with a codebase of this size it's really important that everyone does atomic commits or digging through history becomes a nightmare when we need to.

I really want us to get this done and I'm sure it's going to be a big improvement for Qtile overall, so feel free to come bug me on IRC if you want a faster feedback than here. :)

[ramnes]

LGTM 🚀

[tych0]

After thinking about this some more, I'm still not convinced that this is the right way to solve the problem. It seems the argument in favor of it was "we run the python garbage collector this way, which fixes things". However, we shouldn't need to run the garbage collector: reclaiming memory isn't necessary before exec().

So, this suggests that whatever bug this patch was fixing is a poor use of __del__ or a missing finalize() call somewhere. From the other issue,

I believe this would even cause, for example, an async with statement to not have it's context cleaned up

Shouldn't we be closing this manually on shutdown via the code in the finally block of SessionManager.loop?

[DorianGray]

os.execv would skip finally. It tells the kernel to replace the current process image with a new one.

[tych0]

Yes, but read the code -- the finally block happens before the exec().

[DorianGray]

Now I have no idea what you're talking about. Can you provide some context? snippets?

[tych0]

Sorry, I'd hoped,

in the finally block of SessionManager.loop

was enough:

qtile/libqtile/core/session_manager.py

Line 50 in f3add88

finally: