Follow-up from "Add SkipError for indicating a skipped activity"
The following discussion from !765 (merged) should be addressed:
-
@Qinusty started a discussion: (+1 comment) I found the following comment confusing as the shutdown is currently WITHIN the context.
EDIT: Pasting the relevant comment inline here:
So I git annotate'ed back to where this comment was already violated but the actual statement was outside of the context manager, here: https://gitlab.com/BuildStream/buildstream/blob/0b5809c4b5901df60fa6a3d9e1aa12b9c13872ee/buildstream/_scheduler/job.py#L302
The comment makes sense to me, I'm not sure it's handled in the correct way.
It may be more prudent to block SIGTERM inside of _child_shutdown() (using _signals.blocked()), and also move this outside of the above context manager. Instead of calling _child_shutdown() in multiple locations, we should hold on to a ret variable and call _child_shutdown(ret) once outside of the context managers, that would make more sense.