From 98c1546357b4913024ec9a0a63b69053ae8e920c Mon Sep 17 00:00:00 2001 From: Tristan Van Berkom Date: Wed, 5 Dec 2018 15:08:23 +0900 Subject: _scheduler/queues: Don't call update state outside of error handling harness Commit 3fa79d8da, part of an initiative for caching of the failed builds, introduced a call to Element._update_state() after a job completes and before entering the error handling harness intended for handling plugin raised errors. Element._update_state() can result in triggering plugin code to run, so this is incorrect, and causes raised errors to crash BuildStream if they happen here. After analyzing the code, it appears that this additional call to Element._update_state() is unneeded, and was only added because the state needs to be updated for a failure as well as a success. Instead, we now have the BuildQueue call Element._assemble_done() unconditionally, regardless of whether the build was successful or not, which has the same effect and also reads better. In addition, added a FIXME comment that we are still conditionally updating the artifact cache size from BuildQueue.done() only if the build is successful, which is incorrect because failed builds also increase the local artifact cache size - to fix this we need to communicate the added artifact size through Element._assemble() regardless of whether the build succeeded or failed. --- buildstream/_scheduler/queues/buildqueue.py | 16 +++++++++++----- buildstream/_scheduler/queues/queue.py | 1 - 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/buildstream/_scheduler/queues/buildqueue.py b/buildstream/_scheduler/queues/buildqueue.py index 984a5457a..d05327557 100644 --- a/buildstream/_scheduler/queues/buildqueue.py +++ b/buildstream/_scheduler/queues/buildqueue.py @@ -106,10 +106,16 @@ class BuildQueue(Queue): def done(self, job, element, result, success): - if success: - # Inform element in main process that assembly is done - element._assemble_done() + # Inform element in main process that assembly is done + element._assemble_done() - # This has to be done after _assemble_done, such that the - # element may register its cache key as required + # This has to be done after _assemble_done, such that the + # element may register its cache key as required + # + # FIXME: Element._assemble() does not report both the failure state and the + # size of the newly cached failed artifact, so we can only adjust the + # artifact cache size for a successful build even though we know a + # failed build also grows the artifact cache size. + # + if success: self._check_cache_size(job, element, result) diff --git a/buildstream/_scheduler/queues/queue.py b/buildstream/_scheduler/queues/queue.py index 909cebb44..055e2f84b 100644 --- a/buildstream/_scheduler/queues/queue.py +++ b/buildstream/_scheduler/queues/queue.py @@ -292,7 +292,6 @@ class Queue(): # See the Job object for an explanation of the call signature # def _job_done(self, job, element, success, result): - element._update_state() # Update values that need to be synchronized in the main task # before calling any queue implementation -- cgit v1.2.1