scheduler: remove status field in favor of cancel and invalidation fields

[pmwhite]

Based on #10518

Moves the cancellation token out of the status field, and then removes the status field, since any information that it provides is already present elsewhere.

[rgrinberg]

I wonder if this should go the other way? Why should we have a cancellation token active while we aren't building anything.

[pmwhite]

A subsequent PR is going to remove the status field altogether, since it doesn't add any new information on top of whether the current invalidation is empty and whether the cancellation token has fired or not.

So I guess my proposal is to eliminate the "standing by" state. We should only have two states: "building" and "cancelling build". Which state we are in is determined by whether or not the cancellation token has fired.

Maybe I should just add that commit to this PR instead of having them be separate?

[pmwhite]

To directly answer the question you asked: I agree that we shouldn't have a cancellation token while we are not building, but I want to make it so that there is always a current build (which we might be in the process of cancellation). I think this is a simpler world because there are fewer invalid states to think about.

[rgrinberg]

but I want to make it so that there is always a current build (which we might be in the process of cancellation). I think this is a simpler world because there are fewer invalid states to think about.

What about in watch mode when we are waiting for notifications? What would be the "current build" in that state?

[pmwhite]

The current build would be the most recent build. I'll admit that's a bit a weird from the user's perspective - it makes more sense to say that if the build system is idling, then it isn't in the middle of a build. But in the implementation, it seems to require less code if we don't worry about the build status.

[rgrinberg]

It's less code, but I suspect that's because we don't have to think about the case when you're cancelling a build when we're standing by. Given the bugs that we've been running into, I don't think that's the right trade-off.

IMO, we should go the other way and get rid of the cancel field in the scheduler in favor of cancellation being a part of the build state. It might be more code, but it makes it easier to use the cancellation token correctly.

[pmwhite]

I just combined this PR with the one after it, since this discussion will be most fruitful in the context of actually removing the status field, instead of merely moving the cancellation token. If I weren't also trying to removing the status field, I think I would probably agree that cancellation might as well go inside the status field.

What's weird to me about having an explicit build status is that it is redundant. The memo invalidation and cancellation token contain enough information to reconstruct the build status. Unless it is inefficient or especially awkward, I generally try to avoid representing the same data twice, since that introduces the possibility for the two representations to get out of sync.

It's less code, but I suspect that's because we don't have to think about the case when you're cancelling a build when we're standing by. Given the bugs that we've been running into, I don't think that's the right trade-off.

I don't think not having to think about a case necessarily implies that the case is more likely to be buggy. To the contrary, I've often found that eliminating redundant information causes bugs to be either impossible or more noticeable (and thus more likely to be caught by tests).

IMO, we should go the other way and get rid of the cancel field in the scheduler in favor of cancellation being a part of the build state. It might be more code, but it makes it easier to use the cancellation token

This would mean we can't get rid of the status field, which was my motivation for these refactors. But I agree that if we do keep the status field, we might as well put the cancellation token there and only there. We can go in that direction instead if I fail to convince you that getting rid of the status field is a good idea.

[rgrinberg]

So I agree broadly that the duplicating having two ways of representing the same thing is bad in general for the reason that you've mentioned - syncing is quite error prone. So in that sense, your PR is an improvement. However, before we decide what the code should look like, we should agree on the behavior of cancel_current_build when there's no build running.

As far as I can tell, your PR subtly changes the behavior in the following case:

1. Dune starts in watch mode
2. Finishes a build iteration
3. A call to Scheduler.cancel_current_build () is made

Before this PR, the behavior was to effectively to ignore the cancellation in 3.
After this PR, even though the build is finished, we will now fire the callback corresponding to the Build_interrupted event.

I haven't too hard about this, but I think the previous behavior was better. Why do you like the new behavior more?

On a related note, what would you expect the following pseudo to do:

Scheduler.go (fun () ->
  let* () = Scheduler.cancel_current_build () in
  Build_system.run (fun () -> ...))

The current behavior is that cancellation will make the build not even start. IMO, that's confusing and the cancellation should be a no-op or provide an informative return value that no build is active in such a situation. This is why I don't really like always having a cancellation token available.

[pmwhite]

As far as I can tell, your PR subtly changes the behavior in the following case:

Indeed, I think you're right. This was not intentional, though...I agree that the previous behavior is better, so it seems that it's actually NOT true that we can reconstruct status from cancel and invalidation.

This is why I don't really like always having a cancellation token available.

Yeah, I agree that current behavior of your pseudo code is confusing; my understanding is that this confusion exists both before and after this PR. To fix it, I think we need to somehow have Build_system.run setup a new build, instead of only poll_iter being the point where builds are set up.

[pmwhite]

I wrote an alternative to this PR: #10548. This new one seems superior to me for a couple reasons:

• It doesn't entirely get rid of the status field, since, as you pointed out, doing so would change behavior.
• It picks apart into separate commits the bits of this PR that are actually fairly independent.