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

session: Become subreaper & end children upon finish #374

Merged
merged 1 commit into from
Apr 8, 2021

Conversation

palinek
Copy link
Contributor

@palinek palinek commented Apr 7, 2021

This is another approach to "correctly stop all children upon session exit":

  1. upon start become a subreaper (a process which become a parent of its descendants if its immediate parent ends)
  2. upon session finish send TERM to all children processes and give them a bit time to finish gracefully

This logic is currently implemented only for Linux (proper if/ifdefs used), but based on goole-fu, there are possibilities to implement this for BSD also
https://stackoverflow.com/questions/62365978/get-a-list-of-child-processes-from-parent-process-in-c-and-c-cross-platform
https://unix.stackexchange.com/questions/149319/new-parent-process-when-the-parent-process-dies

@jsm222 Could you have a look for the BSD changes?

@tsujan
Copy link
Member

tsujan commented Apr 7, 2021

I only found some time to test it quickly on my old laptop. I did the tests with FeatherPad, by launching it from lxqt-panel, pcmanfm-qt and by using keyboard shortcut (lxqt-globalkeys) and leaving its window open when logging out. In all three cases, it was terminated correctly on logout (and saved what it should have saved to its config file). Very nice!

Copy link
Member

@tsujan tsujan left a comment

Choose a reason for hiding this comment

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

The code is good too. I'd like it to be in 0.17 if there's no objection.

@palinek palinek force-pushed the lead_and_stop_children branch from 089280f to 348821a Compare April 8, 2021 07:18
@palinek
Copy link
Contributor Author

palinek commented Apr 8, 2021

I'd like it to be in 0.17 if there's no objection.

No objection from my side (of course :) )

I just changed the sleep interval in ProceReaper thread...as there is no need to wake so often, when we use the waitcond and wake the thread instantly upon finish

$ git diff
diff --git a/lxqt-session/src/procreaper.cpp b/lxqt-session/src/procreaper.cpp
index 7c7512c..bb92196 100644
--- a/lxqt-session/src/procreaper.cpp
+++ b/lxqt-session/src/procreaper.cpp
@@ -31,7 +31,7 @@ void ProcReaper::run()
         if (pid <= 0)
         {   
             QMutexLocker guard{&mMutex};
-            mWait.wait(&mMutex, std::chrono::milliseconds(200));
+            mWait.wait(&mMutex, std::chrono::seconds(1));
         }

         int status;

@tsujan
Copy link
Member

tsujan commented Apr 8, 2021

.... as there is no need to wake so often

200 ms didn't have a tangible effect here but, yes,1 sec seems better.

@palinek palinek force-pushed the lead_and_stop_children branch 2 times, most recently from 600ca8d to e347084 Compare April 8, 2021 07:31
@palinek
Copy link
Contributor Author

palinek commented Apr 8, 2021

There was even next typo with variable hiding :( ... fixed

$ git diff
diff --git a/lxqt-session/src/procreaper.cpp b/lxqt-session/src/procreaper.cpp
index bb92196..a8a56b8 100644
--- a/lxqt-session/src/procreaper.cpp
+++ b/lxqt-session/src/procreaper.cpp
@@ -35,7 +35,7 @@ void ProcReaper::run()
         }

         int status;
-        pid_t pid = ::waitpid(-1, &status, WNOHANG);
+        pid = ::waitpid(-1, &status, WNOHANG);
         if (pid < 0)
         {   
             if (ECHILD != errno)

Copy link
Member

@luis-pereira luis-pereira left a comment

Choose a reason for hiding this comment

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

Tested in Linux. It performed as described.
@palinek Consider adding a license notice in each source file.

@palinek palinek force-pushed the lead_and_stop_children branch from e347084 to d198b91 Compare April 8, 2021 20:44
@palinek
Copy link
Contributor Author

palinek commented Apr 8, 2021

Consider adding a license notice in each source file.

Done.

@palinek
Copy link
Contributor Author

palinek commented Apr 8, 2021

Should I merge this?

@tsujan
Copy link
Member

tsujan commented Apr 8, 2021

Should I merge this?

Please do! It's good news for 0.17.

@palinek palinek merged commit d198b91 into master Apr 8, 2021
@palinek palinek deleted the lead_and_stop_children branch April 8, 2021 21:03
@yan12125
Copy link
Member

yan12125 commented Apr 9, 2021

This breaks applications that are expected to live longer than the GUI session (e.g., tmux).

Steps to reproduce:

  1. Run qterminal from within LXQt (click the quicklaunch button in my case)
  2. Run tmux
  3. Detach the tmux session. Now the tmux server is a sub-process of lxqt-session. Here are partial outputs of pstree:
        │                        └─lxqt-session─┬─
        │                                       ├─tmux: server───zsh
  1. Log out of the LXQt session

Expected result: the tmux session persists and can be attached later

Actual result: the tmux session is gone

@tsujan
Copy link
Member

tsujan commented Apr 9, 2021

@yan12125
Any reason that you didn't open an issue?

@palinek
Copy link
Contributor Author

palinek commented Apr 9, 2021

How can the session distinguish which process is supposed to live longer than the GUI session?
Either we should implement some mechanism to leave particular children untouched upon ending session or the user should run such processes outside of session.

@tsujan
Copy link
Member

tsujan commented Apr 10, 2021

How can the session distinguish which process is supposed to live longer than the GUI session?

It can't, unless there's an "inside info" somewhere.

The problem might be avoided if we can add a method for the user to exclude some apps in a new section of LXQt Session Settings. Also, a few apps may be excluded in the code. Just a vague suggestion...

@yan12125 yan12125 mentioned this pull request Apr 10, 2021
@yan12125
Copy link
Member

Any reason that you didn't open an issue?

Argh, possibly because I was sleepy. Created #376.

yan12125 pushed a commit to archlinuxcn/repo that referenced this pull request Apr 10, 2021
Ref: lxqt/lxqt-session#374

Also updates the AUR package from archlinuxcn
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.

4 participants