scheduler: simplify build status handling #10548
Conversation
| if Fiber.Cancel.fired cancellation | ||
| then [] | ||
| else ( | ||
| t.handler t.config Build_interrupted; |
There was a problem hiding this comment.
Before, we'd call this callback unconditionally. Is there a change of behavior here?
There was a problem hiding this comment.
Before, it was impossible to hit this case twice in the same build because of the t.status <- Not_building line above. But without that line, we can hit this case multiple times, and we only want to sent the Build_interrupted notification on the first interruption (instead of sending it on every subsequent interruption).
pmwhite
force-pushed
the
simplify-build-status-handling
branch
from
ad19bb9
to
b29d354
Compare
Signed-Off-By: Philip White <code@trailingwhite.space>
Now that we use Memo.Invalidation.is_empty an indicator that the build should immediately restart, we no longer need a separate state for restarting a build. Signed-Off-By: Philip White <code@trailingwhite.space>
This we count the period after cancellation but before the fiber is completed as part of the build, rather than as Not_building. It also makes it more obvious where state transitions happen. Signed-Off-By: Philip White <code@trailingwhite.space>
Instead of storing the same cancellation token in two places, we always use it from the status field, which allows us to enforce that it is only accessed when there is a build going on. Signed-Off-By: Philip White <code@trailingwhite.space>
pmwhite
force-pushed
the
simplify-build-status-handling
branch
from
b29d354
to
b1f2ddd
Compare
|
I just ran this PR against our internal test suite and hit the following bug: In passive watch mode, if we cancel a build at about the same time as a file changes, then our explicit cancellation will be ignored because it look like the build was cancelled in response to a memo invalidation. I'm going to close this PR for now because I'm no longer sure this is getting me any closer to my goal. |
Based off #10518
This is an alternative to #10519, based on some discussion in that PR. This one keeps the build status intact, but merges
Standing_byandRestarting_buildinto a single case. I recommend reading the diff commit-by-commit.