lxqt / lxqt-session

The LXQt session manager
https://lxqt.github.io
GNU Lesser General Public License v2.1
57 stars 52 forks source link

Restart failed modules with exit codes other than 0 #406

Closed tsujan closed 2 years ago

tsujan commented 2 years ago

Previously, the exit code was ignored when restarting modules.

Recently, after a suspicious libx11 commit, lxqt-globalkeysd may randomly fail at login without crash (see https://github.com/lxqt/lxqt-globalkeys/issues/247). This patch works around the problem but, in general, I found no reason to ignore the exit code — unless I'm missing something…

NOTE: I didn't add a new text for the warning message because, as far as the user is concerned, a sudden exit isn't different from a crash. Also, allowing for 5 extra restarts for the exit code 1 independently of probable crashes seemed too much.

stefonarch commented 2 years ago

So I've seen recently once the message from the panel about failing registering XF86xyz (but they worked after, probably first n attemempts failed) and once the message "Module globalkeys crashed too often and will not be started again until next session", both on cold boots.

tsujan commented 2 years ago

and once the message "Module globalkeys crashed too often...."

It means that globalkeys exited 5 times and the situation of the new libx11 1.7.3.1 may be worse than we thought. Am I dreaming? ;) I can't count the number of my reboots, logins and startups since that report; still, I haven't seen a single failure.

Anyway, this PR has a more general purpose. It seems logical to me, unless @palinek and @luis-pereira tell me that I've missed something.

stefonarch commented 2 years ago

Am I dreaming? ;) I can't count the number of my reboots, logins and startups since that report; still, I haven't seen a single failure.

Yes, you should really find out what's wrong with your system ;)

tsujan commented 2 years ago

Yes, you should really find out what's wrong with your system ;)

Believe me or not, I really think something should be wrong here. There have been too many reports.

EDIT: I intentionally don't use this patch, in order to see the issue.

palinek commented 2 years ago

Btw for too many crashes...we could add a (configurable?) delay between restarts.

tsujan commented 2 years ago

Btw for too many crashes...we could add a (configurable?) delay between restarts.

But we only allow for 5 crashes and, IMO, that's reasonable: if a module crashes more, the problem couldn't be tolerated by restarting it.

stefonarch commented 2 years ago

For the globalkeys module issue now maybe a short delay of 100ms or similar could be appropriated. I don't think we have many restarts usually,

tsujan commented 2 years ago

Here, we're completely in the dark and can't know whether a delay would have a good effect; on the contrary, it might have a negative effect. For example, in the case of globalkeys, a delay might just cause error messages.

luis-pereira commented 2 years ago

I'm still running libX11 1.7.2-1 can't reproduce exits.

tsujan commented 2 years ago

I do agree with @palinek about adding the exit code to the existing log message.

Added now. Please check.

I'm still running libX11 1.7.2-1 can't reproduce exits.

That's expected. What isn't expected is my situation: I have libx11 1.7.3.1 without any problem.

tsujan commented 2 years ago

I think a point release may be needed. It wasn't about a problem, let alone a critical one, but it might compensate for the new X11 issue, although not completely.

stefonarch commented 2 years ago

I agree. Atm in ubuntu 22.04 LTS package is still libx11-6 (2:1.7.2-2) X11 client-side library (and LXQt 0.17), but both could change, the latter should change ;)