Skip to content

Unify and simplify the error handling#4718

Merged
aalekseyev merged 6 commits intoocaml:mainfrom
aalekseyev:unify-and-simplify-error-handling
Jun 10, 2021
Merged

Unify and simplify the error handling#4718
aalekseyev merged 6 commits intoocaml:mainfrom
aalekseyev:unify-and-simplify-error-handling

Conversation

@aalekseyev
Copy link
Copy Markdown
Collaborator

@aalekseyev aalekseyev commented Jun 9, 2021 •
edited
Loading

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 aalekseyev@janestreet.com

@aalekseyev aalekseyev requested a review from rgrinberg June 9, 2021 15:47
@aalekseyev aalekseyev force-pushed the unify-and-simplify-error-handling branch from b1ddd1d to 42fdd3b Compare June 9, 2021 16:04
@aalekseyev
When suppressing errors during cancellation, pattern-match on the exc…
8a64efd 
…eption

instead of inspecting the scheduler state.

Signed-off-by: Arseniy Alekseyev <aalekseyev@janestreet.com>
@aalekseyev aalekseyev force-pushed the unify-and-simplify-error-handling branch from 42fdd3b to 7266e18 Compare June 9, 2021 16:11
aalekseyev added 2 commits June 9, 2021 17:14
@aalekseyev
Prevent the Build_cancelled errors from being reported over the RPC.
246293f 
Signed-off-by: Arseniy Alekseyev <aalekseyev@janestreet.com>
@aalekseyev
Move the error reporting logic from scheduler.ml to build_system.ml
a206c8e 
to unify the two sides.

Signed-off-by: Arseniy Alekseyev <aalekseyev@janestreet.com>
@aalekseyev aalekseyev force-pushed the unify-and-simplify-error-handling branch from 7266e18 to a206c8e Compare June 9, 2021 16:17
@aalekseyev
Copy link
Copy Markdown
Collaborator Author

aalekseyev commented Jun 9, 2021

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

@aalekseyev
fix the bug in [caused_by_cancellation]
8033e5f 
Signed-off-by: Arseniy Alekseyev <aalekseyev@janestreet.com>
@aalekseyev
Copy link
Copy Markdown
Collaborator Author

aalekseyev commented Jun 9, 2021

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

@aalekseyev
ocamlformat
2ff5b1f 
Signed-off-by: Arseniy Alekseyev <aalekseyev@janestreet.com>
@rgrinberg
Copy link
Copy Markdown
Member

rgrinberg commented Jun 9, 2021

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
Copy link
Copy Markdown
Collaborator Author

aalekseyev commented Jun 10, 2021 •
edited
Loading

@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
ghost reviewed Jun 10, 2021
View reviewed changes
src/dune_engine/build_system.ml
match exn.exn with
| Scheduler.Run.Build_cancelled -> true
| Memo.Error.E err -> (
match Memo.Error.get err with
Copy link
Copy Markdown

@ghost ghost Jun 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Collaborator Author

@aalekseyev aalekseyev Jun 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
ghost approved these changes Jun 10, 2021
View reviewed changes
Copy link
Copy Markdown

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@aalekseyev aalekseyev merged commit 30ff3b2 into ocaml:main Jun 10, 2021
@aalekseyev
Copy link
Copy Markdown
Collaborator Author

aalekseyev commented Jun 10, 2021 •
edited
Loading

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

@ghost ghost mentioned this pull request Jun 14, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rgrinberg rgrinberg Awaiting requested review from rgrinberg

1 more reviewer
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@aalekseyev @rgrinberg