Improve thread-safety of GTK/AppIndicator backends

[simonlindholm]

I've observed some intermittent segfaults in a program that calls update_menu() from the setup thread. This seems to be because update_menu calls self._appindicator.set_menu() and _create_menu() off the main thread, while per documentation GTK is not threadsafe. (I'm guessing the problem is mostly with set_menu(), but _create_menu() seems sane to move to the main thread as well.)

The first two commits in this PR fixes the thread-safety, and with them I can no longer reproduce the crashes. The third commit ("Avoid calling accessors twice") isn't necessary to avoid segfaults, but it could avoid some potential Python race condition crashes/incorrect displays, and improve performance microscopically.

The PR includes some minor changes to darwin/win32 backends which have not been tested, and I haven't tested the GTK backend changes either other than to the extent that they don't crash on startup (but it should be fine given that all changed code is shared with AppIndicator).

[moses-palmer]

Thank you very much for your contribution!

I took the liberty to slightly modify your PR in the following way:

• I removed commit 5a2f9f0. I do not think that performance improvements are reason enough to complicate the code, and potential race conditions still exist as any mutation of Icon.menu will cause asynchronous calls to the update methods.
• I rearranged the commits slightly.

The modified commits retain your authorship. I have pushed the modified branch to fixup-gtk-thread-safety. If you agree to my modifications, I will merge it.

[simonlindholm]

The modifications look good to me!

I can understand wanting to drop commit 5a2f9f0, in the sense that the API is kinda inherently racy anyway. However, it does include a fix for a real crash bug:

if self.menu is not None:
    ... for menu_item in self.menu.items ...

This code can run at any time, and if the library user's code picks the wrong moment to set .menu = None it will crash. I think that fix would be good to include. (Or if not, the menu setter ought to check for this case explicitly.)

The other changes in that commit are more cosmetic. They do fix bugs in the sense that they avoid rendering the UI in ways that cannot happen without a race condition (e.g. when the title is shown in two places it could show two different texts for it, or it might show an empty systray menu), but I can understand not wanting to complicate the code for the sake of this.

[moses-palmer]

These changes have been merged to master.

[simonlindholm]

Thanks! Small correction: in commit 1fdb010e6635e6b78ff5e5b72c091cced1923ad5, there is the new addition:

menu_items = self.menu.items if self.menu is not None else None

This is still crash-prone -- a context switch can occur after self.menu is first read and before self.menu.items is evaluated. self.menu needs to be assigned to a local variable.

[moses-palmer]

Yeah, you're correct.

I will remove this modification altogether---this library is not written with any thread safety in mind, as you have noted. In version 1.0, I will remove the menu setter and expect users to rely on the dynamic menu capabilities.

[simonlindholm]

I will remove this modification altogether---this library is not written with any thread safety in mind, as you have noted.

Yeah, I'm growing to realize that when looking at the macOS and win32 implementations of _update_menu, which don't look the least bit thread-safe either... It would be good to fix that, because update_menu makes the library much more powerful (my application is adding/removing items based on network requests/timers, which would be impossible without it... unless I start/stop the main loop on each update, I guess, but I'm not sure how viable that is, it would probably cause a lot of flicker). Unfortunately I don't have access to Windows och Mac machines to help implement that. :(

[moses-palmer]

Just to clarify—I have no intention to remove pystray.Icon.update_menu, but rather the setter for pystray.Icon.menu.

I will then update the documentation to emphasise the intended way to generate dynamic menus—using callables instead of constantly setting the menu. By doing this, pystray will pull data from your application, and no library internal synchronisation is necessary. If your use shared resources to generate the menu, you may need to protect those, but if not—and my assumption is that this is the common case when writing Python applications—a lot of complexity is avoided.

I hope this comment sheds a little light on the design decisions of this library :-)

[simonlindholm]

It was more of an observation on the same theme, but good to hear you won't be removing update_menu! I'll open a separate issue for the macOS/win32 thread unsafety.

In case it's useful with some user feedback, I'm actually using the following equivalent of the .menu setter in my application:

menu_items: List[pystray.MenuItem] = []
icon = pystray.Icon(
    ... menu=pystray.Menu(lambda: menu_items),
)

def inner(icon):
    nonlocal menu_items
    ...
    menu_items = create_items()
    icon.update_menu()
    ....

icon.run(inner)    

It makes menu updates atomic and thread-safety a non-issue for my application code. Passing create_items to the pystray.Menu constructor would feel much too scary for me. I can certainly see how the callback approach is simpler in cases where the menu updates based on checkbox state and such, though, which requires message queues with my setup.

As another point of feedback, my main gripe with the library design is that it's impossible to update a menu item label without refreshing the whole menu, thus closing it if the user has it open, at least on Ubuntu. (The reason I want to update menu item texts is to include some continuously updating statistics in the menu, using a menu item with enabled=False.) I imagine fixing that is out of scope for the library given the current design, though. Other than that and the thread-safety issues (and signal.signal(signal.SIGINT, signal.SIG_DFL) breaking finally blocks with Ctrl-C on Linux, but I can work around that), pystray is working pretty well for me.