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

[Chase Douglas]

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.

-- Chase