Skip to content

Conversation

troycurtisjr
Copy link
Member

Here is my functional, but not code complete, implementation of replacing web sockets over loopback network interface with unix sockets, to address #456 . I still want to do some more local stress testing before I say it is really ready, and I haven't even touched tests yet, but I wanted to post now to get some feedback on the approach. Also this is fully usable, with no changes needed to extensions, so anyone can try it out as well.

Since Ulauncher is already a Gtk based application, and thus has an included GLib event loop, I decided to leverage the capabilities included with GLib. This helps to push parts of the code down into the C library implementation of GLib, vs implementing the unix sockets with the python socket library. Unix sockets allow for better security since they can be created in user directories, with user permissions, thus leveraging the normal file system permissions for protection. Also, unix sockets have a bit less overhead even then local sockets, though this is unlikely to make a big impact in the Ulauncher case.

As part of making use of the event loop, I went on a bit of a thread slaying campaign, converting what I could from threads to events in the event loop. The hope was reduced potential race conditions and hopefully some reduced overhead. However, there are definitely a few run_async calls left that seem to be the easiest implementation, and unfortunately the Python threading library basically auto spawns a pool of threads on first use, so still a big pile of (presumably mostly idle) threads spawn off. Even so, I think many of the places I was able to convert into an event loop will generally help with the multi-thread nuances that can trip up code, so eliminating what I could still makes sense.

Looking at memory consumption of the main Ulauncher process, and of the extension processes, I see about a 40% memory reduction. I'd be curious if that varies with different environments. The savings don't look quite as nice if the preferences dialog is opened because doing so more than doubles the memory usage, which isn't reclaimed when it is closed. This makes the memory savings a much smaller percentage.

I'm interested in feedback on this approach. Also, I recognize some of the increased usage of the GLib event loop is not strictly related to the unix socket implementation, so for the ultimate "real" PR I can make it more focused if you think it is more appropriate. Then I can propose some of the other conversions in separate PRs.

A quick overview of the changes:

  • ulauncher.utils.framer.PickleFramer: The WebSocket implementation provided the "framing" of the pickled objects, so I had to leverage a different mechanism to enclose each pickled event. I went with the standard XDR approach of a "bytes to follow" word, and then the data. This object also handles the pickling part and uses GLib signals for decoupling.
  • ulauncher.api.shared.socket_path: Used the standard xdg runtime location for the unix socket path. On my system this is in "/run/user/"
  • ulauncher.api.shared.event.RegisterEvent: The websocket implementation leveraged the path component of the initial connection to communicate the extension_id, so I had to create a new event to communicate this instead. The main process waits for this first register event before recording the extension as connected, since it doesn't know which extension connected until this point.
  • Extension Server and Client: The overall flow is much the same, just with websockets swapped out with the Gio based unix socket service.
  • ExtensionRunner: I swapped out the use of threads to launch and monitor processes with the Gio.Subprocess equivalent. This eliminates threads, but also hopefully further centralizes management of child processes, which necessarily tend to interact with SIGCHLD and trying to manage subprocesses from multiple threads can have pitfalls (since really only one thread "gets" the SIGCHILD).
  • I'm not sure exactly which of the threads was causing the issues with Ctrl-C not working correctly without running Gtk.main() in a separate thread, but with the ones I eliminated I was also able to drop that work around. I think it might have been the websocket server thread, but I'd have to do more experiments to confirm. Either way, the work around can be dropped.

Sorry for the long message, but I thought it was good to provide the context and overview. As a final thought, I'm happy that this PR has a net removal of more than 400 lines, while increasing performance, reducing memory, and enhancing security. 😀

@friday
Copy link
Member

friday commented Oct 3, 2021

It looks good. I haven't tested it, and I don't know enough to fully understand it without reading up on things. But the code looks clean.

One thing I noticed is that you inlined the tests in timer.py rather than putting them in an external file. Is that a temporary thing? I assume the __main__ condition runs it only in pytest, but not Ulauncher? I don't have any strong opinions on this, and can probably be convinced if you think this is better, but I'm leaning towards keeping tests isolated as they are now.

@troycurtisjr
Copy link
Member Author

oh yes absolutely the tests should be in an external file, those lines were more helping out with my prototyping vs being real tests.

- Remove the self-referencing `./setup.py --version` in __init__.py.
- Remove WebSocketServer and related modules
- Use Gio.SocketService implementation for the new unix sockets.
- Use the new framer module to handle message framing and serializing over unix stream
sockets.
- Make more use of GLib signal/slot functionality for decoupling.
- Drop the now unnecessary extra thread for Gtk.main()
@troycurtisjr troycurtisjr changed the base branch from dev to v6 October 31, 2021 19:01
@troycurtisjr troycurtisjr changed the title Unix Socket Prototype Unix Socket Implementation Oct 31, 2021
- Make sure the right Levenshtein module is installed
- Drop the unnecessary Framer.__str__ override.
@troycurtisjr troycurtisjr marked this pull request as ready for review October 31, 2021 19:22
return ""


__data_directory__ = f'{Path(__file__).resolve().parent.parent}/data'
__is_dev__ = True
__version__ = _exec_get_("git", "describe", "--tags") or _exec_get_("./setup.py", "--version") or "DEV"
__version__ = _exec_get_("git", "describe", "--tags") or "DEV"
Copy link
Member

@friday friday Oct 31, 2021

Choose a reason for hiding this comment

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

git describe --tags assumes the version is a git repo. So if it's not it will be "DEV"
This means for example if someone downloads the tarball source and build it will load "DEV" as the version. Maybe that's fair though.

Copy link
Member Author

@troycurtisjr troycurtisjr Oct 31, 2021

Choose a reason for hiding this comment

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

yeah the important bit though is that ./setup.py --version looks at ulauncher.__init__.__version__ which tries to run ./setup.py --version which looks at ulauncher/__init__.__version__.... and so on until your memory is exhausted ;)

In my case git describe --tags didn't work because I use work trees, and the dev container fails because it can only see the worktree and not the full tree. But no matter the reason, if git describe fails, we can't then call ./setup.py --version, because the only time that can work is:

  • git describe actually works, but then we wouldn't have this problem to start with (e.g ./setup.py --version doesn't get called)
  • __version__ was already overwritten with a static value, in which case we didn't need to call ./setup.py --version in the first place

Copy link
Member

Choose a reason for hiding this comment

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

I see. I figured out how to load the version from setup.cfg with configparser instead, so I can avoid those circular calls. I'll try to fix that later and ask you to review it so I won't sneak in another bug for you 🤦

Copy link
Member

@friday friday left a comment

Choose a reason for hiding this comment

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

This is a huge improvement 🎉
More than 10mb ram savings per extension.

@friday friday merged commit bfa3c5a into v6 Nov 1, 2021
@friday friday deleted the unix-sockets branch November 1, 2021 12:02
@friday friday mentioned this pull request Mar 29, 2022
@friday friday added this to the 6.0.0 milestone Apr 17, 2022
friday added a commit that referenced this pull request Aug 1, 2022
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.

2 participants