Unify and simplify the error handling

[aalekseyev]

This PR does the following:

• commit 1:
  • To detect (and ignore) errors caused by cancellation, we pattern-match on the exception instead of inspecting the scheduler state. This means that it is now possible to keep seeing errors during cancellations, but those errors are "real" errors so are not problematic.
  • Since we have that information in the exception, we don't need all these report_error callbacks whose purpose was to check if we're being cancelled.
• commit 2:
  • Use that information to suppress such errors from being reported over RPC
• commit 3:
  • Move all error handling from Scheduler.poll to Build_system.run so that we don't have to keep synchronizing the two modules on how they handle errors.
  • As a result, the non-polling build errors are now reported by Build_system.run, not by the top-level handler in bin/main.ml, which seems like a good unification.

Signed-off-by: Arseniy Alekseyev [email protected]

[aalekseyev]

@rgrinberg, I borrowed ideas from #4716, but going somewhat further and doing some clean-up of the report_error mess.

[aalekseyev]

Well, my version of caused_by_cancellation was equally buggy. Just tested it and pushed the fix.

[rgrinberg]

Looks good. Could you also implement sending Interrupt and Fail events instead of Finish whenever it makes sense. That will fully replace my PR then.

[aalekseyev]

@rgrinberg, as I said on that PR, I don't understand why that change is necessary or why it's implemented that way.
Is that somehow necessitated by this bugfix, or is it a separate improvement?

[ghost]

To avoid dealing with the optional wrapping, we could change Memo as follow:

• make handle_error_no_raise take a Memo.Error.t and a backtrace rather than an Exn_with_backtrace.t
• make Memo.Build.run* only raise Memo.Error.E

[aalekseyev]

I think it is already the case that the top-level Memo.Build.run* only ever raise Memo.Error.E. We could probably make them return it instead of raising it, even.

For the first bullet-point, agreed. I was even thinking of including the backtrace into Memo.Error.t, but that's a separate idea.

(I'm assuming you're saying these as ideas for the future PRs, so I'm merging this one)

[ghost]

The changes look correct to me and the resulting code looks simpler.

[aalekseyev]

@rgrinberg, I'm merging it because I want it to simplify the other PR. I hope it doesn't break the RPC.