[PATCH 2/2] Use one thread for payload setup.

Brian C. Lane bcl at redhat.com
Mon Aug 25 15:27:08 UTC 2014


On Mon, Aug 25, 2014 at 09:34:17AM -0400, David Shea wrote:
> On 08/22/2014 06:19 PM, Brian C. Lane wrote:
> >On Fri, Aug 22, 2014 at 12:25:38PM -0400, David Shea wrote:
> >
> >This looks really nice, just a few comments below:
> >
> >>+    def addListener(self, event_id, func):
> >>+        """Add a listener for an event.
> >>+
> >>+           :param int event_id: The event to listen for, one of the EVENT_* constants
> >>+           :param function func: An object to call when the event is reached
> >>+        """
> >>+
> >>+        # Check that the event_id is valid
> >>+        assert isinstance(event_id, int)
> >>+        assert event_id <= self.STATE_FINISHED
> >>+        assert event_id >= self.STATE_ERROR
> >>+
> >>+        # Add the listener inside the lock in case we need to run immediately,
> >>+        # to make sure the listener isn't triggered twice
> >>+        with self._event_lock:
> >>+            self._event_listeners[event_id].append(func)
> >>+
> >>+            # If an error event was requested, run it if currently in an error state
> >>+            if event_id == self.STATE_ERROR:
> >>+                if event_id == self._thread_state:
> >>+                    func()
> >>+            # Otherwise, run if the requested event has already occurred
> >>+            elif event_id <= self._thread_state:
> >>+                func()
> >I think you can drop the first block of this if, since the elif is <=
> >it's going to run when the current state == event_id
> 
> The first block is there because STATE_ERROR is -1, and if you add a error
> handler you don't want it running if the thread is in a normal, positive
> number state. I don't know that there's any way around handling the error
> state specially. If it were a big number instead of a small one there would
> need to be a check that installing handlers for the non-error states aren't
> triggered by a payload thread in an error state.

Oh, duh.

> >>      def _initialize(self):
> >>          """ Private initialize. """
> >>-        threadMgr.wait(THREAD_STORAGE)
> >>          threadMgr.wait(THREAD_PAYLOAD)
> >Why drop the wait on storage here? I don't have a specific reason to
> >keep it, just wondering why.
> >
> 
> I don't think it was ever needed, actually. Waiting on THREAD_STORAGE was
> always part of THREAD_PAYLOAD.

Ok, cool. Ack to these then.

-- 
Brian C. Lane | Anaconda Team | IRC: bcl #anaconda | Port Orchard, WA (PST8PDT)


More information about the anaconda-patches mailing list