Bug 81380 - race condition when terminating screensaver
Summary: race condition when terminating screensaver
Status: RESOLVED FIXED
Alias: None
Product: kscreensaver
Classification: Miscellaneous
Component: general (show other bugs)
Version: unspecified
Platform: Compiled Sources Linux
: NOR normal
Target Milestone: ---
Assignee: kscreensaver bugs tracking
URL:
Keywords:
: 81255 (view as bug list)
Depends on:
Blocks:
 
Reported: 2004-05-11 23:40 UTC by Rick Alther
Modified: 2008-05-19 17:59 UTC (History)
2 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Rick Alther 2004-05-11 23:40:06 UTC
Version:            (using KDE KDE 3.2.2)
Installed from:    Compiled From Sources
OS:                Linux

I am writing a screen saver which needs to clean up its resources (open files, child processes, etc.) when the screen saver exits.

I have a SIGTERM handler installed so I get notified when kdesktop_lock ends.  However, there is a race condition with kdesktop_lock: main() may end before my screen saver process cleanly ends.  When main() ends, it destroys the LockProces object which in turn calls the mHackProc destructor, which sends a SIGKILL to my screen saver if it's still running (i.e. I haven't yet finsished cleaning up my resources).

Here's what's happening when the user unlocks the screen:
1) User unlocks screen.
2) This causes an activated() signal on the QSocketNotifier.  The slot is called (sigtermPipeSignal()).
3) sigtermPipeSignla() calls quitSaver()
4) quitSaver() calls stopSaver()
5) stopSaver() calls stopHack()
6) stopHack() sends a SIGTERM to the my screensaver process via kill().
   6a) my screensaver sigterm handler is called initiating cleanup.
7) return back to quitSaver().  quits the KApplicaiton.
8) KApplication event loop ends.  main() ends.
9) Destructors called for stack objects.  Important one being LockProcess
10) LockProcess destroys member objects - Key one being the mHackProc destructor.  This destructor will send a SIGKILL signal to the screensaver process if it's still running.  That's the problem, if I'm still running, it means I'm still cleaning up and it just kills me in the middle of it, potentially leaving corrupt files, orphaned processes, etc.

The Solution:
The solution is to wait for the child process to end before calling kApp->quit().  This can be done either in stopHack() right after the kill() call or the check can be made in the LockProcess destructor

e.g. use the following code:
KProcessController* kpc = KProcessController::theKProcessController;

while ( mHackProc.isRunning() ) {
   kpc->waitForProcessExit(10);
}


There is a potential problem if the mHackProc process never ends (e.g. a bug in the screen saver SIGTERM/cleanup code).  We could modify the the above code so that if the process hasn't exited within a certain amount of time, we just SIGKILL it so the user can get back to their desktop.  In this case, it would be a bug in the running screensaver.

e.g. 
KProcessController* kpc = KProcessController::theKProcessController;

kpc->waitForProcessExit(10);
// We could then issue a mHackProc.kill(SIGKILL) call; 
// - OR -
// If implemented in the destructor, simply let the DTOR run to completion and the 
// KProcess destructor will issue the call for us.
Comment 1 Lubos Lunak 2004-05-19 11:23:14 UTC
*** Bug 81255 has been marked as a duplicate of this bug. ***
Comment 2 Lubos Lunak 2004-05-19 11:24:18 UTC
#81255 is basically the same, about notification when the screensaver exits.
Comment 3 Tvrtko A. Ursulin 2004-05-19 11:38:35 UTC
One additional idea. Maybe KScreenSaver class should support some additional flags when invoking the screensaver so that one can differentiate between real saver start, and saver preview. Why you might ask? Because my screensaver turns off the LCD display on my laptop. It happend even when selecting it in configuration module which is really not-so-cool. :)
Comment 4 Lubos Lunak 2004-05-19 13:25:49 UTC
I personally don't think things like turning LCD display off belong to the actual screensavers. The screensavers are just programs painting silly things. Maybe it'd be better if there was some kind of notification about starting/finishing screensaving.

Comment 5 Rick Alther 2004-05-19 15:21:45 UTC
I agree with Lubos.  Power management should be configured as part of the overall desktop/OS, not as part of a specific screen saver.  If I configure the monitor to power off after 2 hours, I expect the monitor/LCD to power off after two hours of inactivity, regardless what screensaver I have configured (or even if I don't have a screen saver configured).

My suggested solution is a quick and easy one and fits easily into the existing code with no design change.  Once this is implemented, the screen saver process can properly shut down.  If you're really interested, you can tell if you are running as a screen saver or not by comparing the size() of the window you are drawing to to the screen size.  If they're the same, you're in screen saver mode, if not, you're in demo mode (or running in some other window).  Not the most pretty way of doing things, but it can all be done fairly easily.

That doesn't mean there isn't a better way to do this, but a different way of doing things likely requires a slight design change and more work. 
Comment 6 Oswald Buddenhagen 2007-04-12 20:01:32 UTC
SVN commit 653142 by ossi:

give the hack a few seconds to exit.
BUG: 81380


 M  +4 -0      lockprocess.cc  


--- branches/KDE/3.5/kdebase/kdesktop/lock/lockprocess.cc #653141:653142
@@ -857,6 +857,10 @@
     if (mHackProc.isRunning())
     {
         mHackProc.kill();
+        if (!mHackProc.wait(10))
+        {
+            mHackProc.kill(SIGKILL);
+        }
     }
 }