Merge lp:~roadmr/checkbox/588539 into lp:checkbox

[Daniel Manrique]

Modified frontend/backend interaction and FIFO handling so that:

1) The frontend doesn't hang if the backend fails to start
2) The frontend returns "empty" if the frontend hasn't responded to a message in a while
3) The frontend explicitly asks the backend to end when the test run finishes.

This takes care directly of bug 588539. By tweaking the interactive behavior when the backend fails to respond, it should also lead to an eventual fix for bug 334893.

(Rev. 921 - Fixed according to Marc's comments, and refactored some poorly-written (by me) code).

[Marc Tardif (cr3)]

First, you might like to use the default parameter when declaring the timeout property in the BackendInfo class rather than create an additional entry in examples/checkbox.ini. Also, I think you want to declare it as an Int rather than a String property variable.

Second, you forgot a pdb statement in the stop message handler.

review: Needs Fixing

[lp:~roadmr/checkbox/588539]

922. By Daniel Manrique on 2011-06-03

removing stray and unneeded config entry

[Marc Tardif (cr3)]

Looks good, merging and building package.

review: Approve

=== modified file 'backend'
--- backend	2010-04-20 16:23:16 +0000
+++ backend	2011-06-03 20:51:05 +0000
@@ -28,6 +28,8 @@
     while True:
         try:
             message = reader.read_object()
+            if message == "stop":
+                break
             if isinstance(message, dict) and "command" in message:
                 job = Job(message["command"], message.get("environ"),
                     message.get("timeout"))

=== added file 'checkbox/lib/enum.py'
--- checkbox/lib/enum.py	1970-01-01 00:00:00 +0000
+++ checkbox/lib/enum.py	2011-06-03 20:51:05 +0000
@@ -0,0 +1,72 @@
+#
+# This file is part of Checkbox.
+#
+# Copyright 2011 Canonical Ltd.
+#
+# Checkbox is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# Checkbox is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with Checkbox.  If not, see <http://www.gnu.org/licenses/>.
+#
+__all__ = [
+    "EnumException", "Enum", "enum_name_to_value", "enum_value_to_name"]
+
+
+class EnumException(Exception):
+
+    pass
+
+
+class Enum(object):
+
+    def __init__(self, *names):
+        value = 0
+        unique_names = set()
+        for name in names:
+            if isinstance(name, (list, tuple)):
+                name, value = name
+
+            if not isinstance(name, str):
+                raise EnumException(
+                    "enum name is not a string: %s" % name)
+
+            if isinstance(value, str):
+                if value not in unique_names:
+                    raise EnumException(
+                        "enum value does not define: %s" % value)
+                value = getattr(self, value)
+
+            if not isinstance(value, (int, long)):
+                raise EnumException(
+                    "enum value is not an integer: %s" % value)
+
+            if name in unique_names:
+                raise EnumException(
+                    "enum name is not unique: %s" % name)
+
+            unique_names.add(name)
+            setattr(self, name, value)
+            value += 1
+
+
+def enum_name_to_value(enum, name):
+    if not hasattr(enum, name):
+        raise EnumException("enum name unknown: %s" % name)
+
+    return getattr(enum, name)
+
+
+def enum_value_to_name(enum, value):
+    for k, v in enum.__dict__.items():
+        if v == value:
+            return k
+
+    raise EnumException("enum value unknown: %s" % value)

=== modified file 'checkbox/lib/fifo.py'
--- checkbox/lib/fifo.py	2010-03-28 03:00:14 +0000
+++ checkbox/lib/fifo.py	2011-06-03 20:51:05 +0000
@@ -20,15 +20,16 @@
 import struct
 
 from checkbox.contrib.bpickle import dumps, loads
-
+from checkbox.lib.selector import Selector, SelectorIO
 
 class FifoBase(object):
 
     mode = None
 
-    def __init__(self, path):
+    def __init__(self, path, timeout=None):
         self.path = path
         self.file = open(path, self.mode)
+        self._timeout = timeout
 
     def __del__(self):
         self.close()
@@ -36,12 +37,29 @@
     def close(self):
         self.file.close()
 
+    def wait_for(self, operation):
+        if self._timeout is not None:
+            selector = Selector()
+            selector.set_timeout(self._timeout)
+            selector.add_fd(self.file.fileno(), operation)
+
+            selector.execute()
+
+            if not selector.has_ready():
+                return False
+        return True
 
 class FifoReader(FifoBase):
 
-    mode = "r"
+    #on Linux, opening a FIFO in read-write mode is non-blocking and
+    #succeeds even if other end is not open as per FIFO(7)
+    mode = "w+"
 
     def read_string(self):
+        # Check if a connection arrived within the timeout
+        if not self.wait_for(SelectorIO.READ):
+            return None
+
         size = struct.calcsize("i")
         length_string = self.file.read(size)
         if not length_string:
@@ -60,9 +78,16 @@
 
 class FifoWriter(FifoBase):
 
-    mode = "w"
+    #on Linux, opening a FIFO in read-write mode is non-blocking and
+    #succeeds even if other end is not open as per FIFO(7)
+    mode = "w+"
 
     def write_string(self, string):
+
+        # Wait until I can write 
+        if not self.wait_for(SelectorIO.WRITE):
+            return None 
+
         length = len(string)
         length_string = struct.pack(">i", length)
         self.file.write(length_string)

=== added file 'checkbox/lib/selector.py'
--- checkbox/lib/selector.py	1970-01-01 00:00:00 +0000
+++ checkbox/lib/selector.py	2011-06-03 20:51:05 +0000
@@ -0,0 +1,207 @@
+#
+# This file is part of Checkbox.
+#
+# Copyright 2011 Canonical Ltd.
+#
+# Checkbox is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# Checkbox is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with Checkbox.  If not, see <http://www.gnu.org/licenses/>.
+#
+import os
+import errno
+import select
+
+from checkbox.lib.enum import Enum
+
+
+__all__ = ["SelectorIO", "SelectorState", "Selector"]
+
+
+SelectorIO = Enum(
+    "READ",
+    "WRITE",
+    "EXCEPT")
+
+SelectorState = Enum(
+    "VIRGIN",
+    "READY",
+    "TIMED_OUT",
+    "SIGNALED",
+    "FAILED")
+
+
+class Selector(object):
+    __slots__ = ("_read_fds", "_write_fds", "_except_fds",
+        "_save_read_fds", "_save_write_fds", "_save_except_fds",
+        "_fd_set_size", "_timeout", "_state", "_errno")
+
+    _fd_select_size = -1
+    _cached_read_fds = None
+    _cached_save_read_fds = None
+    _cached_write_fds = None
+    _cached_save_write_fds = None
+    _cached_except_fds = None
+    _cached_save_except_fds = None
+
+    def __init__(self):
+        if Selector._cached_read_fds:
+            self._read_fds = Selector._cached_read_fds
+            self._write_fds = Selector._cached_write_fds
+            self._except_fds = Selector._cached_except_fds
+
+            self._save_read_fds = Selector._cached_save_read_fds
+            self._save_write_fds = Selector._cached_save_write_fds
+            self._save_except_fds = Selector._cached_save_except_fds
+
+            Selector._cached_read_fds = None
+            Selector._cached_write_fds = None
+            Selector._cached_except_fds = None
+
+            Selector._cached_save_read_fds = None
+            Selector._cached_save_write_fds = None
+            Selector._cached_save_except_fds = None
+
+        else:
+            self._read_fds = []
+            self._write_fds = []
+            self._except_fds = []
+
+            self._save_read_fds = []
+            self._save_write_fds = []
+            self._save_except_fds = []
+
+        self.reset()
+
+    def __del__(self):
+        if Selector._cached_read_fds is None:
+            Selector._cached_read_fds = self._read_fds
+            Selector._cached_write_fds = self._write_fds
+            Selector._cached_except_fds = self._except_fds
+
+            Selector._cached_save_read_fds = self._save_read_fds
+            Selector._cached_save_write_fds = self._save_write_fds
+            Selector._cached_save_except_fds = self._save_except_fds
+
+    def reset(self):
+        self._errno = 0
+        self._state = SelectorState.VIRGIN
+        self._timeout = None
+        self._save_read_fds = []
+        self._save_write_fds = []
+        self._save_except_fds = []
+
+    @staticmethod
+    def get_fd_select_size():
+        if Selector._fd_select_size == -1:
+            try:
+                Selector._fd_select_size = os.sysconf("SC_OPEN_MAX")
+            except (AttributeError, ValueError):
+                Selector._fd_select_size = 1024
+
+        return Selector._fd_select_size
+
+    def get_errno(self):
+        return self._errno
+
+    def get_errstr(self):
+        return os.strerror(self._errno)
+
+    def set_timeout(self, timeout):
+        self._timeout = timeout
+
+    def unset_timeout(self):
+        self._timeout = None
+
+    def has_timed_out(self):
+        return self._state == SelectorState.TIMED_OUT
+
+    def has_signaled(self):
+        return self._state == SelectorState.SIGNALED
+
+    def has_failed(self):
+        return self._state == SelectorState.FAILED
+
+    def has_ready(self):
+        return self._state == SelectorState.READY
+
+    def add_fd(self, fd, interest):
+        if fd < 0 or fd >= Selector.get_fd_select_size():
+            raise Exception("File descriptor %d outside of range 0-%d"
+                % (fd, Selector._fd_select_size))
+
+        if interest == SelectorIO.READ:
+            self._save_read_fds.append(fd)
+
+        elif interest == SelectorIO.WRITE:
+            self._save_write_fds.append(fd)
+
+        elif interest == SelectorIO.EXCEPT:
+            self._save_except_fds.append(fd)
+
+    def remove_fd(self, fd, interest):
+        if fd < 0 or fd >= Selector.get_fd_select_size():
+            raise Exception("File descriptor %d outside of range 0-%d"
+                % (fd, Selector._fd_select_size))
+
+        if interest == SelectorIO.READ:
+            self._save_read_fds.remove(fd)
+
+        elif interest == SelectorIO.WRITE:
+            self._save_write_fds.remove(fd)
+
+        elif interest == SelectorIO.EXCEPT:
+            self._save_except_fds.remove(fd)
+
+    def execute(self):
+        try:
+            self._read_fds, self._write_fds, self._except_fds = select.select(
+                self._save_read_fds, self._save_write_fds,
+                self._save_except_fds, self._timeout)
+        except select.error, e:
+            self._errno = e.errno
+            if e.errno == errno.EINTR:
+                self._state = SelectorState.SIGNALED
+
+            else:
+                self._state = SelectorState.FAILED
+
+            return
+
+        # Just in case
+        self._errno = 0
+        if not self._read_fds \
+           and not self._write_fds \
+           and not self._except_fds:
+            self._state = SelectorState.TIMED_OUT
+
+        else:
+            self._state = SelectorState.READY
+
+    def is_fd_ready(self, fd, interest):
+        if self._state != SelectorState.READY \
+           and self._state != SelectorState.TIMED_OUT:
+            raise Exception(
+                "Selector requested descriptor not in ready state")
+
+        if fd < 0 or fd >= Selector.get_fd_select_size():
+            return False
+
+        if interest == SelectorIO.READ:
+            return fd in self._read_fds
+
+        elif interest == SelectorIO.WRITE:
+            return fd in self._write_fds
+
+        elif interest == SelectorIO.EXCEPT:
+            return fd in self._except_fds
+
+        return False

=== modified file 'debian/changelog'
--- debian/changelog	2011-05-25 14:10:15 +0000
+++ debian/changelog	2011-06-03 20:51:05 +0000
@@ -3,11 +3,14 @@
   [Daniel Manrique]
   * Fix GUI definition file so main window uses "natural request", growing
     when child widgets require so (LP: #776734)
+  * Fix open/read blocking behavior and backend/frontend communications to
+    avoid hangs and lingering backends. (LP: #588539)
 
   [Robert Roth]
   * Improve command line key prompts (LP: #786924)
 
- -- Marc Tardif <[e-mail]>  Wed, 25 May 2011 10:09:22 -0400
+  
+ -- Daniel Manrique <[e-mail]>  Fri, 03 Jun 2011 15:23:05 -0400
 
 checkbox (0.12) oneiric; urgency=low
 

=== modified file 'plugins/backend_info.py'
--- plugins/backend_info.py	2011-05-24 19:15:37 +0000
+++ plugins/backend_info.py	2011-06-03 20:51:05 +0000
@@ -25,11 +25,15 @@
 from checkbox.lib.fifo import FifoReader, FifoWriter, create_fifo
 
 from checkbox.plugin import Plugin
-from checkbox.properties import Path
+from checkbox.properties import Path, Float 
 
 
 class BackendInfo(Plugin):
 
+    # how long to wait for I/O from/to the backend. If it takes longer
+    # than this, the message is ignored.
+    timeout = Float(default=15.0)
+
     command = Path(default="%(checkbox_share)s/backend")
 
     def register(self, manager):
@@ -76,8 +80,8 @@
 
         self.pid = os.fork()
         if self.pid > 0:
-            self.parent_writer = FifoWriter(child_input)
-            self.parent_reader = FifoReader(child_output)
+            self.parent_writer = FifoWriter(child_input, timeout=self.timeout)
+            self.parent_reader = FifoReader(child_output, timeout=self.timeout)
 
         else:
             root_command = self.get_root_command(child_input, child_output)
@@ -92,6 +96,7 @@
                 self._manager.reactor.fire("message-result", *result)
 
     def stop(self):
+        self.parent_writer.write_object("stop")
         self.parent_writer.close()
         self.parent_reader.close()
         shutil.rmtree(self.directory)