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.

Edited by Tristan Van Berkom