[PATCH xorg-gtest 06/16] xserver: move Terminate and Kill handling here

[Peter Hutterer]

On Mon, Jul 09, 2012 at 04:59:02PM -0700, Chase Douglas wrote:
> On 07/09/2012 04:26 PM, Peter Hutterer wrote:
> >On Mon, Jul 09, 2012 at 10:19:50AM -0700, Chase Douglas wrote:
> >>On 07/08/2012 06:50 PM, Peter Hutterer wrote:
> >>>On Fri, Jul 06, 2012 at 11:08:13AM -0700, Chase Douglas wrote:
> >>>>On 07/05/2012 05:50 PM, Peter Hutterer wrote:
> >>>>>On Thu, Jul 05, 2012 at 02:09:13PM -0700, Chase Douglas wrote:
> >>>>>>On 07/03/2012 10:42 PM, Peter Hutterer wrote:
> >>>>>>>On Tue, Jul 03, 2012 at 10:33:50AM -0700, Chase Douglas wrote:
> >>>>>>>>On 07/02/2012 11:44 PM, Peter Hutterer wrote:
> >>>>>>>>>Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> >>>>>>>>>---
> >>>>>>>>>  include/xorg/gtest/xorg-gtest-xserver.h |   13 ++++++++++++
> >>>>>>>>>  src/environment.cpp                     |   31 +++-------------------------
> >>>>>>>>>  src/xserver.cpp                         |   34 +++++++++++++++++++++++++++++++
> >>>>>>>>>  3 files changed, 50 insertions(+), 28 deletions(-)
> >>>>>>>>>
> >>>>>>>>>diff --git a/include/xorg/gtest/xorg-gtest-xserver.h b/include/xorg/gtest/xorg-gtest-xserver.h
> >>>>>>>>>index 52a2fd0..821b01f 100644
> >>>>>>>>>--- a/include/xorg/gtest/xorg-gtest-xserver.h
> >>>>>>>>>+++ b/include/xorg/gtest/xorg-gtest-xserver.h
> >>>>>>>>>@@ -53,6 +53,19 @@ class XServer : public xorg::testing::Process {
> >>>>>>>>>      void Start(std::string &program);
> >>>>>>>>>
> >>>>>>>>>      /**
> >>>>>>>>>+     * Terminates this server process. Will signal the server to terminate
> >>>>>>>>>+     * multiple times before giving up.
> >>>>>>>>>+     *
> >>>>>>>>>+     * @return false if the server did not terminate, true otherwise
> >>>>>>>>>+     */
> >>>>>>>>>+    bool Terminate(void);
> >>>>>>>>>+
> >>>>>>>>>+    /**
> >>>>>>>>>+     * Kills the server. With a vengeance.
> >>>>>>>>>+     */
> >>>>>>>>>+    bool Kill(void);
> >>>>>>>>
> >>>>>>>>We don't need to recreate these functions. We've already inherited
> >>>>>>>>them from xorg::testing::Process. Those implementations should work
> >>>>>>>>automatically if we set up the XServer class properly.
> >>>>>>>
> >>>>>>>The implementation is different to the one in Process, this one does the
> >>>>>>>server-specific bits like waiting for the process to shut down and then
> >>>>>>>complaining to the log.
> >>>>>>
> >>>>>>I read too quickly to realize the difference between the
> >>>>>>Environment/XServer implementation and the Process implementation,
> >>>>>>but I'm wondering if we should just move the extra stuff to the
> >>>>>>Process implementation. Then we won't need to do any redefinition or
> >>>>>>overriding.
> >>>>>
> >>>>>I think we should keep Process basic. For most processes failing to kill it
> >>>>>should be an issue in itself, it's just the server that takes too long to
> >>>>>shut down and needs special handling.
> >>>>
> >>>>I thought about that too. I think the question to ask is: do we
> >>>>think most users or subclasses of Process are going to want wait for
> >>>>the process to really die. The Process::Kill and Terminate methods
> >>>>just try once to raise the signal to the child process. They return
> >>>>true if the signal was sent, but don't check what happens after
> >>>>that. Perhaps a better approach to resolving this issue would be to
> >>>>add second versions of these methods that would take a timeout value
> >>>>and return true only if the signal was successfully sent and the
> >>>>process died.
> >>>>KillAndCheck(timeout) and TerminateAndCheck(timeout) maybe?
> >>>
> >>>maybe so, but let's worry about that when we have other users of Process
> >>>that need this feature in a generic parent class?
> >>
> >>I can't argue with that :). Feel free to leave them in the XServer
> >>object for now, though I'd prefer if you renamed them to something
> >>like KillAndCheck so it's obvious that it actually does something
> >>different than the parent class Kill method.
> >
> >but isn't that the point of polymorphism? your object will do the right
> >thing, regardless of what class it happens to be at the time.
> 
> The difference here is that we are doing two different things.
> Process::Kill() merely signals the process. XServer::KillAndCheck()
> signals the process and waits for it to die. Using polymorphism to
> hide implementation differences between classes is useful. Using
> polymorphism to hide behavior changes is dangerous.
> 
> Let's say you define XServer::Kill(), so that it waits a second to
> check if it has died. Now, let's say there is a function that is
> passed a Process object and calls Kill() on it. If the object is
> simply a Process object, Kill() will return immediately after
> signalling the process. If the object is an XServer project, it will
> only return after waiting for up to a second. This could create odd
> interactions due to the hidden behavior difference depending on the
> actual type of the object.
> 
> What qualifies as a behavior difference vs an implementation
> difference isn't black and white, but this particular case seems to
> me to clearly fall on the behavior side of the fence.

changed locally, will be in next patch set

Cheers,
  Peter