dreamcat4 / skippy-xd

A full-screen Exposé-style standalone task switcher for X11.
GNU General Public License v2.0
99 stars 12 forks source link

Interprocess communication without using named pipes #39

Closed felixfung closed 1 year ago

felixfung commented 1 year ago

Skippy-xd daemon mode is important in alt-tab (why?) and storing thumbnail of minimized windows. However, current communication with the daemon is done through named pipe, which is not fast enough to handle all the incoming X events.

Implement communication to invoke skippy-xd daemon.

felixfung commented 1 year ago

@dreamcat4 can you please clarify what is the status of named pipe and skippy-runner?

I have not experienced any dropped X events or duplicated keys or unresponsive daemon. And given the new functionality to display unmapped windows, we really wouldn't want to kill the daemon, and hence that would kind of obsolete skippy-runner?

What was the purpose of the daemon mode prior to tracking unmapped windows anyway?

Sorry for repeated questioning on this topic.

felixfung commented 1 year ago

@dreamcat4 sorry for missing the changes on skippy-runner on #66 and thanks for fixing it.

I would like to ask again, given we don't want to kill the daemon (when the daemon dies and restart all unmapped windows becomes boxes), what is the purpose of skippy-runner? We would rather want to strongly prioritize on fixing segfaults?

dreamcat4 commented 1 year ago

I would like to ask again, given we don't want to kill the daemon (when the daemon dies and restart all unmapped windows becomes boxes)

ah yes, since we have your new stuff. This script maybe is killing both instance indiscriminately (in all cases).... which is sub-optimal behaviour.

I may try to improve that aspect now. With some tweaks.

I shall try to be improving my runner script a little bit. If it can do better to avoid killing the daemon (...unless really the daemon has solidly hanged). Well, IDK yet. Leave it to me. Then do a bit of testing on those changes. 1-2 weeks. Will see how that goes.

what is the purpose of skippy-runner?

OK so the purpose was supposed to be clearly written within the file itself:

# A wrapper script to deal with too many requests / locks up skippy
# When skippy is bound to a global keybinding, and held keys repeat
#
# Functions:
#   * Restart skippy daemon if stuck / unresponsive (> 1 sec)
#   * Don't overload skippy with any simultaneous requests
#   * Auto-start daemon for --activate and --toggle commands
#

the 2 most important lines are:

# When skippy is bound to a global keybinding, and held keys repeat
#   * Don't overload skippy with any simultaneous requests

But what the explanation does not mention is that:

With the main processing loop of skippy: it is fundamentally a single threaded program. It goes away to service other request(s). And is blocking the main loop during that time. It must be responding to X-Events and so on. And so time can pass. It may be responding to those, and doing other things. In a complex, and/or non-deterministic fashion.

So (for example) lets imagine: skippy gets invoked by a repeating on the keyboard. OK... I am hammering the keys. The window is cycling fffffffffffffffffffffffffffffffffffffff....

That is a worst-case stress test. But also it happens! (like quite often in real life). If user has 30 windows open. You hold down alt+tab, and it's going to be the system's key repeat rate that dictates the effective rate speed at which skippy --next is being invoked.

And while this is happening, the overall PC... it could be doing anything. Perhaps the CPU is become very busy. Or other times, the WM (for example KDE) is actually HUNG. (and yes, this also happens very often in real world, this KDE after all I am using). Ok then...

But the action of pressing the key itself... it does not just invoke skippy --next. That same action - it also generates a bunch of X-Events. This stream of x-event then also repeats itself again (many times). And a queues builds up. However skippy may then service these subsequent key event out of order with listening on specific x-events....

The answer becomes: Who knows. So yes (despite not being a multi-threaded app).... this is not very predicable stable state of affairs.

So we have this runner script. It is much more conservative program than skippy is. It knows that if the previous client activation request was still running, ok then it will wait for TIMEOUT seconds (by default 1 sec, but is configurable to N secs). Then it waited. But still busy. So probably? something has gotten delayed significantly enough to affect user experience. Because skippy still is not done with previous client... that previous client is still waiting to be serviced by the daemon. Or whatever the reason might be. OK then...

So (as the runner script): I am not going to make the problem worse by trying to invoke skippy again, and generate more work....

This might become improved situation in future, if we were to eventually replacee of unix FIFO sockets with some interprocess IPC mechanism. IDK (but we have not got that yet).

We would rather want to strongly prioritize on fixing segfaults?

Well I can still see any segfaults if they occur, even with the skippy-xd-runner. However whenever they do occur... I also get to have a very graceful and helpful recovery mechanism... you are presenting the question as either / or. But the two things are not actually mutually exclusive. I would rather see such segfault occurs, and still be able to carry on to use my PC, thanks.

Also:

It may not be a realistic belief that (with the historical state of all this previous old code) to actually catch all possible segfaults in every corner of the skippy program.

Like... even if your code added is all perfect. Nevermind what others did before you came along. Great if we can improve... but it's just such a rat's nest to get to 0 bugs....

So this runner script deals with some realistic possibility that in a C program memory allocation bugs may occur from time to time... is not ideal, but we do not live in a perfect world! (and this is a C program.... = manual memory management), so having 1 fallback mechanism is better than 0.

You don't need to use this script.... that is totally fine. Nobody is forcing to use this script. It's perfectly fine to use skippy-xd directly and without runner at all. But it still has an intrinsic value, and it still saves my butt.

I have now answered this question 3 times (about the purpose of skippy-xd-runner). Is that finally a sufficient explanation?

felixfung commented 1 year ago

@dreamcat4 regarding the key hammering issue, wondering if the fixing of #106 has improved the issue? My personal experience of #106 is that it gives the illusion of bogus keystrokes...

felixfung commented 1 year ago

@dreamcat4 with the latest PR I tried key jamming for switch/expose/paging, and I experience no hiccups or any issues at all. Wondering if you are still experiencing any issues on KDE/Budgie?

felixfung commented 1 year ago

@dreamcat4 I have now experimented in KDE, jamming switch, expose, paging, without using skippy-runner, I experience no hiccups or any issues at all. Everything is smooth, no lagging, doesn't get into broken state. (I got slightly dizzy from the visuals though :P )

Can you please try running with/without skippy-runner and report?

dreamcat4 commented 1 year ago

Can you please try running with/without skippy-runner and report?

yes however i feel a necessary work (for myself) here is in addition to also write a proper user space systemd service file. because if skippy daemon crashes for whatever unforseen reason(s). it should be restarted automatically. and that should be fairly simple matter

just that specific item (to create systemd service file). it ended up at bottom of broader list of priorities! so yes - worth to do that next, and then report back after those necessary retest.

however before that i also need some block of time away from this project. to then come back again. so this will be deferred for next time... but is put at top of the list. will be my first thing to do.

anyhow i can at least thank you for being so diligent here to observe / and highlight this matters. just to expect some delay now until it will get dealt with. but my gratitude

dreamcat4 commented 1 year ago

Can you please try running with/without skippy-runner and report?

OK I have tried now. It's part solved. But not fully, as exists a wider situation / matter. Of a prior use case (so not a regression bug, but a regression in observed functionality. Which i would like to re-instate return back into the skippy program. That remaining matter is created as it's own seperated new issue here:

https://github.com/dreamcat4/skippy-xd/issues/161

In terms of removal of runner script, it will be phased removal:

When the systemd user file is added, then the runner script will not any longer to be auto installed anymore (and not generally speaking recommended). And be removed from docs...

So it may be due to be taken out from "make install" target. While the script itself can perhaps linger in the git repo. At least for a while longer.

felixfung commented 1 year ago

I don't understand, you propose introducing systemd service just to catch skippy crash and rerun?

Crashes happen on any app that is written in c/c++. But that doesn't mean we write wrapper around each c/c++ app. The correct approach is to encourage reporting of crashes and its reproduction steps, and then fix the crash. That is because crashes are high impact bugs, but only low/moderate cost to fix. So the priority shouldn't be so much to hide them or to work around them, but to fix them.

I can give you a recent example. While testing on GNOME, skippy would crash on every second run. Quick debugging shows that the root cause was GNOME uses _NET_CLIENT_LIST rather than XQueryTree call in the config option, and I put that in the wiki for GNOME user, and we have perfect resolving of this crash issue. In this case and in general, wrapping skippy for crash would be a fundamentally bad approach.

I am going to close this issue because I don't believe in replacing the named pipe mechanism, or introducing systemd service. If you incline you can remove skippy-runner.

dreamcat4 commented 1 year ago

you propose introducing systemd service just to catch skippy crash and rerun?

Absolutely no:

It is (more generally speaking) an addition which is frequently requested by both distros and regular users. Who both generally prefer this exact file to be included upstream these days. Living in modern era. Rather than have quite a few individual distro(s) reproduce such a file to add it themselves. And then have different version(s) or variants of that service file out there.

Another issue with your argument to discourage systemd is that it only holds a value in the situation of continued project support. Because in the situation of no longer support (to fix bugs) then restarting of the daemon is in fact a more desirable outcome.

The example given is indeed a good counterpoint. However on balance it is too narrow to justify outright prohibiting any such introduction of a systemd service file. Which BTW does include relevant mechanisms to detect any such failures and crashes. (Or not), as is the determination of the end user of the system. But it is not a forced determination.

Furthermore:

The actual usage of the systemd service file is itself not mandatory when installed. However as an alternative to the runner script it is less confusing alternative (from a user perspective, as to which command to actually run to invoke skippy). So that is itself a step forwards.

felixfung commented 1 year ago

I don't 100% agree with you but I do agree that introducing an extra file does not hurt.