[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] server/process: Avoid hanging if a parser process is terminated

[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.

Use signal masks to avoid this.

Some ideas from Peter Kjellerstedt <peter.kjellerstedt@axis.com>

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

diff --git a/lib/bb/server/process.py b/lib/bb/server/process.py
index efc3f04b4c..dc331b3957 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
@@ -739,8 +740,12 @@ class ConnectionWriter(object):
 
     def send(self, obj):
         obj = multiprocessing.reduction.ForkingPickler.dumps(obj)
+        # We must not terminate holding this lock. For SIGTERM, raising afterwards avoids this.
+        # For SIGINT, we don't want to have written partial data to the pipe.
+        signal.pthread_sigmask(signal.SIG_BLOCK, (signal.SIGINT, signal.SIGTERM))
         with self.wlock:
             self.writer.send_bytes(obj)
+        signal.pthread_sigmask(signal.SIG_UNBLOCK, (signal.SIGINT, signal.SIGTERM))
 
     def fileno(self):
         return self.writer.fileno()
-- 
2.32.0

[PATCH 5/6] cooker: Ensure parsing processes have close called

[Richard Purdie]

With pyhon 3.7 there is a close method which collects the process' return
value and ensures zombines don't hang around. Add that call. Also join terminated
processes before closing since the signal might not be instantaneous.

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

diff --git a/lib/bb/cooker.py b/lib/bb/cooker.py
index 2264b18c54..4b8b18a68d 100644
--- a/lib/bb/cooker.py
+++ b/lib/bb/cooker.py
@@ -2198,8 +2198,10 @@ class CookerParser(object):
             if force:
                 process.join(.1)
                 process.terminate()
-            else:
-                process.join()
+            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
-- 
2.32.0

[PATCH 6/6] cooker: Pass SIGINT to parsing processes at shutdown

[Richard Purdie]

To try and improve shutdown, sent SIGINT to parsing processes now it
is handled safely around locks, perhaps avoiding the need for the more
risky SIGTERM.

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

diff --git a/lib/bb/cooker.py b/lib/bb/cooker.py
index 4b8b18a68d..bd7ab8ac3c 100644
--- a/lib/bb/cooker.py
+++ b/lib/bb/cooker.py
@@ -2146,7 +2146,7 @@ class CookerParser(object):
             def init():
                 signal.signal(signal.SIGTERM, signal.SIG_DFL)
                 signal.signal(signal.SIGHUP, signal.SIG_DFL)
-                signal.signal(signal.SIGINT, signal.SIG_IGN)
+                signal.signal(signal.SIGINT, signal.default_int_handler)
                 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)
@@ -2194,6 +2194,9 @@ class CookerParser(object):
             except queue.Empty:
                 break
 
+        for process in self.processes:
+            os.kill(process.pid, signal.SIGINT)
+
         for process in self.processes:
             if force:
                 process.join(.1)
-- 
2.32.0

Re: [bitbake-devel] [PATCH 4/6] server/process: Avoid hanging if a parser process is terminated

[Richard Purdie]

On Sat, 2022-03-26 at 20:34 +0000, Richard Purdie via lists.openembedded.org
wrote:
> 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.
> 
> Use signal masks to avoid this.
> 
> Some ideas from Peter Kjellerstedt <peter.kjellerstedt@axis.com>
> 
> Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
> ---
>  lib/bb/server/process.py | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/lib/bb/server/process.py b/lib/bb/server/process.py
> index efc3f04b4c..dc331b3957 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
> @@ -739,8 +740,12 @@ class ConnectionWriter(object):
>  
>      def send(self, obj):
>          obj = multiprocessing.reduction.ForkingPickler.dumps(obj)
> +        # We must not terminate holding this lock. For SIGTERM, raising afterwards avoids this.
> +        # For SIGINT, we don't want to have written partial data to the pipe.
> +        signal.pthread_sigmask(signal.SIG_BLOCK, (signal.SIGINT, signal.SIGTERM))
>          with self.wlock:
>              self.writer.send_bytes(obj)
> +        signal.pthread_sigmask(signal.SIG_UNBLOCK, (signal.SIGINT, signal.SIGTERM))
>  
>      def fileno(self):
>          return self.writer.fileno()

This doesn't work as you'd expect, see https://bugs.python.org/issue47139 where
I mentioned the issue upstream.

Cheers,

Richard

[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