| Message ID | 20220326203458.1391301-1-richard.purdie@linuxfoundation.org |
|---|---|
| State | Accepted, archived |
| Commit | ae89e23696de2f27c00ae00922933395171de5d5 |
| Headers | show |
| Series | [1/6] cooker: Fix exception handling in parsers | expand |
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
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
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(-)