Re: [PATCH 4/4] xmlrpc: add support for event-observer-only connection

[Richard Purdie <richard.purdie@linuxfoundation.org>]

On Fri, 2013-05-31 at 12:06 +0100, Alex DAMIAN wrote:
> From: Alexandru DAMIAN <alexandru.damian@intel.com>
> 
> This patch adds support for multiple UI clients acting only as event syncs.
> Summary of changes:
> 
> bitbake: adds support for --observe-only command line parameter
> 
> xmlrpc server: create a Observer-only connection type, and small changes
> to accomodate new incoming parameters
> 
> event queue: add exclusive access to structures for multithreaded
> operation; this is needed by accepting new UI handlers on a different thread
> 
> knotty: support for observer-only connections
> 
> Other minor cosmetic changes to support new parameters to functions were made
> 
> Based on original patch by Bogdan Marinescu <bogdan.a.marinescu@intel.com>
> 
> Signed-off-by: Alexandru DAMIAN <alexandru.damian@intel.com>
> ---
>  bin/bitbake             |  8 ++++++-
>  lib/bb/event.py         | 63 ++++++++++++++++++++++++++++++++++---------------
>  lib/bb/server/xmlrpc.py | 57 +++++++++++++++++++++++++++++++++++++-------
>  lib/bb/ui/knotty.py     | 29 +++++++++++++----------
>  lib/bb/ui/uievent.py    |  4 ++--
>  5 files changed, 119 insertions(+), 42 deletions(-)

I merged the other three patches in the series. I was not happy with the
commit message summary as "XXX: fixes for XXX" tells me nothing. In this
case I rewrote them as examples of what looks better but please think
about those a bit more in future.

For this patch I have some concerns.

> diff --git a/bin/bitbake b/bin/bitbake
> index d263cbd..ef0c5d8 100755
> --- a/bin/bitbake
> +++ b/bin/bitbake
> @@ -197,6 +197,9 @@ class BitBakeConfigParameters(cookerdata.ConfigParameters):
>          parser.add_option("", "--remote-server", help = "Connect to the specified server",
>                     action = "store", dest = "remote_server", default = False)
>  
> +        parser.add_option("", "--observe-only", help = "Connect to a server as an observing-only client",
> +                   action = "store_true", dest = "observe_only", default = False)
> +
>          options, targets = parser.parse_args(sys.argv)
>          return options, targets[1:]
>  
> @@ -269,6 +272,9 @@ def main():
>      if configParams.remote_server and configParams.servertype != "xmlrpc":
>          sys.exit("FATAL: If '--remote-server' is defined, we must set the servertype as 'xmlrpc'.\n")
>  
> +    if configParams.observe_only and (not configParams.remote_server or configParams.bind):
> +        sys.exit("FATAL: '--observe-only' can only be used by UI clients connecting to a server.\n")
> +
>      if "BBDEBUG" in os.environ:
>          level = int(os.environ["BBDEBUG"])
>          if level > configuration.debug:
> @@ -295,7 +301,7 @@ def main():
>          server = start_server(servermodule, configParams, configuration)
>      else:
>          # we start a stub server that is actually a XMLRPClient to
> -        server = servermodule.BitBakeXMLRPCClient()
> +        server = servermodule.BitBakeXMLRPCClient(configParams.observe_only)
>          server.saveConnectionDetails(configParams.remote_server)


Do we really need a commandline option for this or is this something the
UIs can figure out and do for themselves?

>      logger.removeHandler(handler)
> diff --git a/lib/bb/event.py b/lib/bb/event.py
> index 2826e35..726d074 100644
> --- a/lib/bb/event.py
> +++ b/lib/bb/event.py
> @@ -33,12 +33,15 @@ import atexit
>  import traceback
>  import bb.utils
>  import bb.compat
> +import threading

I've already mentioned that mixing mutliprocessing and threading in
bitbake has been known to cause problems. Did you investigate those
issues and do we know this is safe here?

I actually don't even understand why we're locking this. Only the server
process should be able to touching this queue.

>  # This is the pid for which we should generate the event. This is set when
>  # the runqueue forks off.
>  worker_pid = 0
>  worker_pipe = None
>  
> +_ui_handlers_lock = threading.Lock()
> +
>  logger = logging.getLogger('BitBake.Event')
>  
>  class Event(object):
> @@ -93,6 +96,8 @@ def fire_class_handlers(event, d):
>              continue
>  
>  ui_queue = []
> +_ui_event_history = []
> +_ui_event_history_lock = threading.Lock()
>  @atexit.register
>  def print_ui_queue():
>      """If we're exiting before a UI has been spawned, display any queued
> @@ -124,22 +129,34 @@ def fire_ui_handlers(event, d):
>          # No UI handlers registered yet, queue up the messages
>          ui_queue.append(event)
>          return
> -
> +    _ui_event_history_lock.acquire()
> +    _ui_event_history.append(event)
> +    _ui_event_history_lock.release()
>      errors = []
> -    for h in _ui_handlers:
> -        #print "Sending event %s" % event
> -        try:
> -             # We use pickle here since it better handles object instances
> -             # which xmlrpc's marshaller does not. Events *must* be serializable
> -             # by pickle.
> -             if hasattr(_ui_handlers[h].event, "sendpickle"):
> -                _ui_handlers[h].event.sendpickle((pickle.dumps(event)))
> -             else:
> -                _ui_handlers[h].event.send(event)
> -        except:
> -            errors.append(h)
> -    for h in errors:
> -        del _ui_handlers[h]
> +    _ui_handlers_lock.acquire()
> +    try:
> +        for h in _ui_handlers:
> +            #print "Sending event %s" % event
> +            try:
> +                 # We use pickle here since it better handles object instances
> +                 # which xmlrpc's marshaller does not. Events *must* be serializable
> +                 # by pickle.
> +                 if hasattr(_ui_handlers[h].event, "sendpickle"):
> +                    _ui_handlers[h].event.sendpickle((pickle.dumps(event)))
> +                 else:
> +                    _ui_handlers[h].event.send(event)
> +            except:
> +                errors.append(h)
> +        for h in errors:
> +            del _ui_handlers[h]
> +    finally:
> +        _ui_handlers_lock.release()
> +
> +def get_event_history():
> +    _ui_event_history_lock.acquire()
> +    evt_copy = _ui_event_history[:]
> +    _ui_event_history_lock.release()
> +    return evt_copy


I'm afraid I don't like this at all. Why do we need to keep event
history? Surely the UIs are meant to query the server for current state,
not rely on a reply of existing events?

Reading the code I can figure out what you're doing and why and answer
my own questions. The commit message however sucks as it doesn't tell me
any of this. The locking and so on also looks inappropriate and
worrisome.

So this patch needs some further thought/work.

Cheers,

Richard