[PATCH 1/6] cooker: Fix exception handling in parsers

[PATCH 1/6] cooker: Fix exception handling in parsers

[Richard Purdie]

We shouldn't be generating exception inside a generator. Rearrange the
code to improve the handling of this. Also fix the misconverted code
from when multiconfig was added and pass the exception as "result".

Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
---
 lib/bb/cooker.py | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/lib/bb/cooker.py b/lib/bb/cooker.py
index eac956aa97..c4d720a6b6 100644
--- a/lib/bb/cooker.py
+++ b/lib/bb/cooker.py
@@ -2087,12 +2087,12 @@ class Parser(multiprocessing.Process):
             tb = sys.exc_info()[2]
             exc.recipe = filename
             exc.traceback = list(bb.exceptions.extract_traceback(tb, context=3))
-            return True, exc
+            return True, None, exc
         # Need to turn BaseExceptions into Exceptions here so we gracefully shutdown
         # and for example a worker thread doesn't just exit on its own in response to
         # a SystemExit event for example.
         except BaseException as exc:
-            return True, ParsingFailure(exc, filename)
+            return True, None, ParsingFailure(exc, filename)
         finally:
             bb.event.LogHandler.filter = origfilter
 
@@ -2252,11 +2252,7 @@ class CookerParser(object):
                 pass
             else:
                 empty = False
-                value = result[1]
-                if isinstance(value, BaseException):
-                    raise value
-                else:
-                    yield result
+                yield result
 
         if not (self.parsed >= self.toparse):
             raise bb.parse.ParseError("Not all recipes parsed, parser thread killed/died? Exiting.", None)
@@ -2267,6 +2263,9 @@ class CookerParser(object):
         parsed = None
         try:
             parsed, mc, result = next(self.results)
+            if isinstance(result, BaseException):
+                # Turn exceptions back into exceptions
+                raise result
         except StopIteration:
             self.shutdown()
             return False
-- 
2.32.0

[PATCH 2/6] cooker: Fix main loop starvation when parsing

[Richard Purdie]

When parsing, the parser isn't servicing the main loop so a Ctrl+C in the
UI won't be seen on the cooker/server side. Fix this by returning when queue
timeouts occur. This helps where there is a hung or slow parsing thread.

Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
---
 lib/bb/cooker.py | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/lib/bb/cooker.py b/lib/bb/cooker.py
index c4d720a6b6..fb71a968f2 100644
--- a/lib/bb/cooker.py
+++ b/lib/bb/cooker.py
@@ -2249,7 +2249,7 @@ class CookerParser(object):
                 result = self.result_queue.get(timeout=0.25)
             except queue.Empty:
                 empty = True
-                pass
+                yield None, None, None
             else:
                 empty = False
                 yield result
@@ -2266,6 +2266,10 @@ class CookerParser(object):
             if isinstance(result, BaseException):
                 # Turn exceptions back into exceptions
                 raise result
+            if parsed is None:
+                # Timeout, loop back through the main loop
+                return True
+
         except StopIteration:
             self.shutdown()
             return False
-- 
2.32.0

[PATCH 3/6] cooker: Improve exception handling in parsing process

[Richard Purdie]

If an exception occurs in the parsing process, ensure the cleanup
is called via all codepaths using a try/finally.

Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
---
 lib/bb/cooker.py | 48 ++++++++++++++++++++++++------------------------
 1 file changed, 24 insertions(+), 24 deletions(-)

diff --git a/lib/bb/cooker.py b/lib/bb/cooker.py
index fb71a968f2..2264b18c54 100644
--- a/lib/bb/cooker.py
+++ b/lib/bb/cooker.py
@@ -2041,32 +2041,32 @@ class Parser(multiprocessing.Process):
             self.init()
 
         pending = []
-        while True:
-            try:
-                self.quit.get_nowait()
-            except queue.Empty:
-                pass
-            else:
-                self.results.close()
-                self.results.join_thread()
-                break
-
-            if pending:
-                result = pending.pop()
-            else:
+        try:
+            while True:
                 try:
-                    job = self.jobs.pop()
-                except IndexError:
-                    self.results.close()
-                    self.results.join_thread()
+                    self.quit.get_nowait()
+                except queue.Empty:
+                    pass
+                else:
                     break
-                result = self.parse(*job)
-                # Clear the siggen cache after parsing to control memory usage, its huge
-                bb.parse.siggen.postparsing_clean_cache()
-            try:
-                self.results.put(result, timeout=0.25)
-            except queue.Full:
-                pending.append(result)
+
+                if pending:
+                    result = pending.pop()
+                else:
+                    try:
+                        job = self.jobs.pop()
+                    except IndexError:
+                        break
+                    result = self.parse(*job)
+                    # Clear the siggen cache after parsing to control memory usage, its huge
+                    bb.parse.siggen.postparsing_clean_cache()
+                try:
+                    self.results.put(result, timeout=0.25)
+                except queue.Full:
+                    pending.append(result)
+        finally:
+            self.results.close()
+            self.results.join_thread()
 
     def parse(self, mc, cache, filename, appends):
         try:
-- 
2.32.0

[PATCH 4/6] cooker: Simplify parser init function handling

[Richard Purdie]

Not sure why this is so convoluted but we should simplify it!

Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
---
 lib/bb/cooker.py | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/lib/bb/cooker.py b/lib/bb/cooker.py
index 2264b18c54..d6fcd9e05c 100644
--- a/lib/bb/cooker.py
+++ b/lib/bb/cooker.py
@@ -2009,11 +2009,10 @@ class ParsingFailure(Exception):
         Exception.__init__(self, realexception, recipe)
 
 class Parser(multiprocessing.Process):
-    def __init__(self, jobs, results, quit, init, profile):
+    def __init__(self, jobs, results, quit, profile):
         self.jobs = jobs
         self.results = results
         self.quit = quit
-        self.init = init
         multiprocessing.Process.__init__(self)
         self.context = bb.utils.get_context().copy()
         self.handlers = bb.event.get_class_handlers().copy()
@@ -2037,8 +2036,12 @@ class Parser(multiprocessing.Process):
             prof.dump_stats(logfile)
 
     def realrun(self):
-        if self.init:
-            self.init()
+        signal.signal(signal.SIGTERM, signal.SIG_DFL)
+        signal.signal(signal.SIGHUP, signal.SIG_DFL)
+        signal.signal(signal.SIGINT, signal.SIG_IGN)
+        bb.utils.set_process_name(multiprocessing.current_process().name)
+        multiprocessing.util.Finalize(None, bb.codeparser.parser_cache_save, exitpriority=1)
+        multiprocessing.util.Finalize(None, bb.fetch.fetcher_parse_save, exitpriority=1)
 
         pending = []
         try:
@@ -2143,13 +2146,6 @@ class CookerParser(object):
         self.processes = []
         if self.toparse:
             bb.event.fire(bb.event.ParseStarted(self.toparse), self.cfgdata)
-            def init():
-                signal.signal(signal.SIGTERM, signal.SIG_DFL)
-                signal.signal(signal.SIGHUP, signal.SIG_DFL)
-                signal.signal(signal.SIGINT, signal.SIG_IGN)
-                bb.utils.set_process_name(multiprocessing.current_process().name)
-                multiprocessing.util.Finalize(None, bb.codeparser.parser_cache_save, exitpriority=1)
-                multiprocessing.util.Finalize(None, bb.fetch.fetcher_parse_save, exitpriority=1)
 
             self.parser_quit = multiprocessing.Queue(maxsize=self.num_processes)
             self.result_queue = multiprocessing.Queue()
@@ -2159,7 +2155,7 @@ class CookerParser(object):
             self.jobs = chunkify(list(self.willparse), self.num_processes)
 
             for i in range(0, self.num_processes):
-                parser = Parser(self.jobs[i], self.result_queue, self.parser_quit, init, self.cooker.configuration.profile)
+                parser = Parser(self.jobs[i], self.result_queue, self.parser_quit, self.cooker.configuration.profile)
                 parser.start()
                 self.process_names.append(parser.name)
                 self.processes.append(parser)
-- 
2.32.0

[PATCH 5/6] cooker/process: Fix signal handling lockups

[Richard Purdie]

If a parser process is terminated while holding a write lock, then it
will lead to a deadlock (see
https://docs.python.org/3/library/multiprocessing.html#multiprocessing.Process.terminate).

With SIGTERM, we don't want to terminate holding the lock. We also don't
want a SIGINT to cause a partial write to the event stream.

I tried using signal masks to avoid this but it doesn't work, see
https://bugs.python.org/issue47139

Instead, add a signal handler and catch the calls around the critical section.
We also need a thread lock to ensure other threads in the same process don't
handle the signal until all the threads are not in the lock.

Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
---
 lib/bb/cooker.py         | 28 ++++++++++++++++++++++++++--
 lib/bb/server/process.py | 22 ++++++++++++++++++++--
 2 files changed, 46 insertions(+), 4 deletions(-)

diff --git a/lib/bb/cooker.py b/lib/bb/cooker.py
index d6fcd9e05c..7c0c5d4efa 100644
--- a/lib/bb/cooker.py
+++ b/lib/bb/cooker.py
@@ -2017,6 +2017,22 @@ class Parser(multiprocessing.Process):
         self.context = bb.utils.get_context().copy()
         self.handlers = bb.event.get_class_handlers().copy()
         self.profile = profile
+        self.queue_signals = False
+        self.signal_received = []
+        self.signal_threadlock = threading.Lock()
+
+    def catch_sig(self, signum, frame):
+        if self.queue_signals:
+            self.signal_received.append(signum)
+        else:
+            self.handle_sig(signum, frame)
+
+    def handle_sig(self, signum, frame):
+        if signum == signal.SIGTERM:
+            signal.signal(signal.SIGTERM, signal.SIG_DFL)
+            os.kill(os.getpid(), signal.SIGTERM)
+        elif signum == signal.SIGINT:
+            signal.default_int_handler(signum, frame)
 
     def run(self):
 
@@ -2036,9 +2052,17 @@ class Parser(multiprocessing.Process):
             prof.dump_stats(logfile)
 
     def realrun(self):
-        signal.signal(signal.SIGTERM, signal.SIG_DFL)
+        # Signal handling here is hard. We must not terminate any process or thread holding the write
+        # lock for the event stream as it will not be released, ever, and things will hang.
+        # Python handles signals in the main thread/process but they can be raised from any thread and
+        # we want to defer processing of any SIGTERM/SIGINT signal until we're outside the critical section
+        # and don't hold the lock (see server/process.py). We therefore always catch the signals (so any
+        # new thread should also do so) and we defer handling but we handle with the local thread lock
+        # held (a threading lock, not a multiprocessing one) so that no other thread in the process
+        # can be in the critical section.
+        signal.signal(signal.SIGTERM, self.catch_sig)
         signal.signal(signal.SIGHUP, signal.SIG_DFL)
-        signal.signal(signal.SIGINT, signal.SIG_IGN)
+        signal.signal(signal.SIGINT, self.catch_sig)
         bb.utils.set_process_name(multiprocessing.current_process().name)
         multiprocessing.util.Finalize(None, bb.codeparser.parser_cache_save, exitpriority=1)
         multiprocessing.util.Finalize(None, bb.fetch.fetcher_parse_save, exitpriority=1)
diff --git a/lib/bb/server/process.py b/lib/bb/server/process.py
index 7c587a9110..ce53fdc678 100644
--- a/lib/bb/server/process.py
+++ b/lib/bb/server/process.py
@@ -20,6 +20,7 @@ import os
 import sys
 import time
 import select
+import signal
 import socket
 import subprocess
 import errno
@@ -737,11 +738,28 @@ class ConnectionWriter(object):
         # Why bb.event needs this I have no idea
         self.event = self
 
-    def send(self, obj):
-        obj = multiprocessing.reduction.ForkingPickler.dumps(obj)
+    def _send(self, obj):
         with self.wlock:
             self.writer.send_bytes(obj)
 
+    def send(self, obj):
+        obj = multiprocessing.reduction.ForkingPickler.dumps(obj)
+        # See notes/code in CookerParser
+        # We must not terminate holding this lock else processes will hang.
+        # For SIGTERM, raising afterwards avoids this.
+        # For SIGINT, we don't want to have written partial data to the pipe.
+        # pthread_sigmask block/unblock would be nice but doesn't work, https://bugs.python.org/issue47139
+        process = multiprocessing.current_process()
+        if process and hasattr(process, "queue_signals"):
+            with process.signal_threadlock:
+                process.queue_signals = True
+                self._send(obj)
+                process.queue_signals = False
+                for sig in process.signal_received.pop():
+                    process.handle_sig(sig, None)
+        else:
+            self._send(obj)
+
     def fileno(self):
         return self.writer.fileno()
 
-- 
2.32.0

[PATCH 6/6] cooker: Rework force parser shutdown

[Richard Purdie]

The "force" option to parser shutdown was often the cause of lockups and
there is no good reason we should have two different behaviours.

Change and unify the codepaths to always:
  * Wait for longer for a controlled shutdown of a process (0.5s). Usually
    it will be much faster if it has finished so the delay doesn't really matter.
  * Send processes a SIGINT
  * Failing that, send a SIGTERM
  * Call .close() if available to release zombies

This means we no longer need the "force" parameter to the function so it is removed.

Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
---
 lib/bb/cooker.py | 37 +++++++++++++++++++++++++------------
 1 file changed, 25 insertions(+), 12 deletions(-)

diff --git a/lib/bb/cooker.py b/lib/bb/cooker.py
index 7c0c5d4efa..f435b18c87 100644
--- a/lib/bb/cooker.py
+++ b/lib/bb/cooker.py
@@ -1613,7 +1613,7 @@ class BBCooker:
 
         if self.state in (state.shutdown, state.forceshutdown, state.error):
             if hasattr(self.parser, 'shutdown'):
-                self.parser.shutdown(clean=False, force = True)
+                self.parser.shutdown(clean=False)
                 self.parser.final_cleanup()
             raise bb.BBHandledException()
 
@@ -1741,7 +1741,7 @@ class BBCooker:
             self.state = state.shutdown
 
         if self.parser:
-            self.parser.shutdown(clean=not force, force=force)
+            self.parser.shutdown(clean=not force)
             self.parser.final_cleanup()
 
     def finishcommand(self):
@@ -2186,7 +2186,7 @@ class CookerParser(object):
 
             self.results = itertools.chain(self.results, self.parse_generator())
 
-    def shutdown(self, clean=True, force=False):
+    def shutdown(self, clean=True):
         if not self.toparse:
             return
         if self.haveshutdown:
@@ -2215,11 +2215,24 @@ class CookerParser(object):
                 break
 
         for process in self.processes:
-            if force:
-                process.join(.1)
+            process.join(0.5)
+
+        for process in self.processes:
+            if process.exitcode is None:
+                os.kill(process.pid, signal.SIGINT)
+
+        for process in self.processes:
+            process.join(0.5)
+
+        for process in self.processes:
+            if process.exitcode is None:
                 process.terminate()
-            else:
-                process.join()
+
+        for process in self.processes:
+            process.join()
+            # Added in 3.7, cleans up zombies
+            if hasattr(process, "close"):
+                process.close()
 
         self.parser_quit.close()
         # Allow data left in the cancel queue to be discarded
@@ -2296,18 +2309,18 @@ class CookerParser(object):
         except bb.BBHandledException as exc:
             self.error += 1
             logger.debug('Failed to parse recipe: %s' % exc.recipe)
-            self.shutdown(clean=False, force=True)
+            self.shutdown(clean=False)
             return False
         except ParsingFailure as exc:
             self.error += 1
             logger.error('Unable to parse %s: %s' %
                      (exc.recipe, bb.exceptions.to_string(exc.realexception)))
-            self.shutdown(clean=False, force=True)
+            self.shutdown(clean=False)
             return False
         except bb.parse.ParseError as exc:
             self.error += 1
             logger.error(str(exc))
-            self.shutdown(clean=False, force=True)
+            self.shutdown(clean=False)
             return False
         except bb.data_smart.ExpansionError as exc:
             self.error += 1
@@ -2316,7 +2329,7 @@ class CookerParser(object):
             tb = list(itertools.dropwhile(lambda e: e.filename.startswith(bbdir), exc.traceback))
             logger.error('ExpansionError during parsing %s', value.recipe,
                          exc_info=(etype, value, tb))
-            self.shutdown(clean=False, force=True)
+            self.shutdown(clean=False)
             return False
         except Exception as exc:
             self.error += 1
@@ -2328,7 +2341,7 @@ class CookerParser(object):
                 # Most likely, an exception occurred during raising an exception
                 import traceback
                 logger.error('Exception during parse: %s' % traceback.format_exc())
-            self.shutdown(clean=False, force=True)
+            self.shutdown(clean=False)
             return False
 
         self.current += 1
-- 
2.32.0