Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix race in starup/restart logic #4757

Merged
merged 1 commit into from
Apr 13, 2024
Merged

Conversation

tych0
Copy link
Member

@tych0 tych0 commented Apr 6, 2024

I am getting:

2024-04-06 13:38:10,669 ERROR libqtile core.py:_xpoll():L372 Got an exception in poll loop Traceback (most recent call last):
File "/home/tycho/.local/lib/python3.11/site-packages/libqtile/backend/x11/core.py", line 347, in _xpoll
self.handle_event(event)
File "/home/tycho/.local/lib/python3.11/site-packages/libqtile/backend/x11/core.py", line 314, in handle_event
ret = target(event)
^^^^^^^^^^^^^
File "/home/tycho/.local/lib/python3.11/site-packages/libqtile/backend/x11/core.py", line 663, in handle_KeyPress
key, handled = self.qtile.process_key_event(keysym, event.state & self._valid_mask)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/tycho/.local/lib/python3.11/site-packages/libqtile/core/manager.py", line 449, in process_key_event
if cmd.check(self):
^^^^^^^^^^^^^^^
File "/home/tycho/.local/lib/python3.11/site-packages/libqtile/lazy.py", line 148, in check
cur_win_floating = q.current_window and q.current_window.floating
^^^^^^^^^^^^^^^^
File "/home/tycho/.local/lib/python3.11/site-packages/libqtile/command/base.py", line 281, in getattr
raise AttributeError(f"{self.class} has no attribute {name}")
AttributeError: <class 'libqtile.core.manager.Qtile'> has no attribute current_window

on restart.

current_screen is first set in:

loop
-> async_loop
-> load_config()
-> _process_screens()

but in async_loop(), we setup_listener() before we load the config, i.e.:

loop
-> async_loop
-> setup_listener() # whoops
...
-> load_config()
-> _process_screens()

This means that we could potentially race and start receiving events before we were completely configured, as above. This is especially likely if we are restarting, but the race exists as well on startup and could be present if e.g. people were starting things in their xsessions before exec()ing qtile.

To fix this, we load the config before we setup the listener, so that everything will be present. But there's a bit of a circular dependency we need to break here: setup_listener() is the one that initialize's the Core's copy of the qtile object. Let's just set that explicitly in Core's async_loop(), and remove the parameter of setup_listener(). So then we have:

  1. initialize qtile object
  2. load_config()
  3. setup_listener()

and hopefully we won't see more races like this. Indeed, what clued me in that it was a race vs just a bug is that I saw other missing attributes sometimes on restarts e.g.:

AttributeError: <class 'libqtile.backend.x11.window.Window'> has no attribute has_focus

This is an interesting bug: it seems like it has been around for quite a long time, but perhaps the new asyncio and python speed improvements are making it such that now we can actually see it?

Fixes #4755

@tych0
Copy link
Member Author

tych0 commented Apr 6, 2024

Arg, mypy really doesn't like:

libqtile/core/manager.py:213: error: "Core" has no attribute "qtile"  [attr-defined]

@tych0 tych0 force-pushed the fix-one-restart-race branch 2 times, most recently from 2ae8e57 to 5c89f9c Compare April 6, 2024 20:28
@tych0
Copy link
Member Author

tych0 commented Apr 6, 2024

Ok, I think I finally got it. But wayland people take a look; I think it simplifies the code greatly to just use Qtile instead of Qtile | None here, but perhaps there's some dependency I don't understand.

@@ -211,7 +211,10 @@ def wmname(self, wmname):
self._wmname = wmname
self._supporting_wm_check_window.set_property("_NET_WM_NAME", wmname)

def setup_listener(self, qtile: "Qtile") -> None:
def set_qtile(self, qtile: "Qtile") -> None:
self.qtile
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't look right.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, thanks. leftover from when I was considering using a setter; I deleted it.

I am getting:

2024-04-06 13:38:10,669 ERROR libqtile core.py:_xpoll():L372 Got an exception in poll loop
Traceback (most recent call last):
  File "/home/tycho/.local/lib/python3.11/site-packages/libqtile/backend/x11/core.py", line 347, in _xpoll
    self.handle_event(event)
  File "/home/tycho/.local/lib/python3.11/site-packages/libqtile/backend/x11/core.py", line 314, in handle_event
    ret = target(event)
          ^^^^^^^^^^^^^
  File "/home/tycho/.local/lib/python3.11/site-packages/libqtile/backend/x11/core.py", line 663, in handle_KeyPress
    key, handled = self.qtile.process_key_event(keysym, event.state & self._valid_mask)
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/tycho/.local/lib/python3.11/site-packages/libqtile/core/manager.py", line 449, in process_key_event
    if cmd.check(self):
       ^^^^^^^^^^^^^^^
  File "/home/tycho/.local/lib/python3.11/site-packages/libqtile/lazy.py", line 148, in check
    cur_win_floating = q.current_window and q.current_window.floating
                       ^^^^^^^^^^^^^^^^
  File "/home/tycho/.local/lib/python3.11/site-packages/libqtile/command/base.py", line 281, in __getattr__
    raise AttributeError(f"{self.__class__} has no attribute {name}")
AttributeError: <class 'libqtile.core.manager.Qtile'> has no attribute current_window

on restart.

current_screen is first set in:

loop
  -> async_loop
    -> load_config()
      -> _process_screens()

but in async_loop(), we setup_listener() before we load the config, i.e.:

loop
  -> async_loop
    -> setup_listener() # whoops
    ...
    -> load_config()
      -> _process_screens()

This means that we could potentially race and start receiving events before
we were completely configured, as above. This is especially likely if we
are restarting, but the race exists as well on startup and could be present
if e.g. people were starting things in their xsessions before exec()ing
qtile.

To fix this, we load the config before we setup the listener, so that
everything will be present. But there's a bit of a circular dependency we
need to break here: setup_listener() is the one that initialize's the
Core's copy of the qtile object. Let's just set that explicitly in
Core's async_loop(), and remove the parameter of setup_listener(). So then
we have:

1. initialize qtile object
2. load_config()
3. setup_listener()

and hopefully we won't see more races like this. Indeed, what clued me in
that it was a race vs just a bug is that I saw other missing attributes
sometimes on restarts e.g.:

    AttributeError: <class 'libqtile.backend.x11.window.Window'> has no attribute has_focus

This is an interesting bug: it seems like it has been around for quite a
long time, but perhaps the new asyncio and python speed improvements are
making it such that now we can actually see it?

Fixes qtile#4755

Signed-off-by: Tycho Andersen <tycho@tycho.pizza>
@tych0
Copy link
Member Author

tych0 commented Apr 13, 2024

I'm gonna merge this since it fixes a real bug. Hopefully it doesn't break the wayland stuff too hard :)

@tych0 tych0 merged commit f987f5f into qtile:master Apr 13, 2024
25 checks passed
@jwijenbergh
Copy link
Contributor

I'm gonna merge this since it fixes a real bug. Hopefully it doesn't break the wayland stuff too hard :)

:( #4767 seems to be related to this

Comment on lines +393 to +394
if hasattr(self, "qtile"):
delattr(self, "qtile")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should have looked at this earlier, my bad. hmmm why is this needed on wayland? On wayland we can't restart

jwijenbergh added a commit to jwijenbergh/qtile that referenced this pull request Apr 15, 2024
In qtile#4757 we have fixed some of the logic with regards to setting
self.qtile. This means that in certain scenarios the self.qtile
attribute does not exist, e.g. when we're adding keyboards befor the
config is loaded.

We already mark these devices as "pending" if
self.qtile is `None`, but this now does not work as the attribute is not
set in the first place. Let's check with hasattr and remove the
unneeded self.qtile setting inside of the keyboard constructor.

This fixes qtile#4767
jwijenbergh added a commit to jwijenbergh/qtile that referenced this pull request Apr 15, 2024
In qtile#4757 we have fixed some of the logic with regards to setting
self.qtile. This means that in certain scenarios the self.qtile
attribute does not exist, e.g. when we're adding keyboards before the
config is loaded.

We already mark these devices as "pending" if
self.qtile is `None`, but this now does not work as the attribute is not
set in the first place. Let's check with hasattr and remove the
unneeded self.qtile setting inside of the keyboard constructor.

This fixes qtile#4767
tych0 pushed a commit that referenced this pull request Apr 15, 2024
In #4757 we have fixed some of the logic with regards to setting
self.qtile. This means that in certain scenarios the self.qtile
attribute does not exist, e.g. when we're adding keyboards before the
config is loaded.

We already mark these devices as "pending" if
self.qtile is `None`, but this now does not work as the attribute is not
set in the first place. Let's check with hasattr and remove the
unneeded self.qtile setting inside of the keyboard constructor.

This fixes #4767
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

has no attribute current_window
3 participants