-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Lin: Print commands that trigger unexpected exceptions #324
base: main
Are you sure you want to change the base?
Conversation
This is both a nice improvement and elegantly implemented - many thanks! 😃
|
I finally got round to update that PR with your suggestions. It also takes care now to preserve the original backtrace. |
Well, that’s quite a success... |
The Furthermore, the distribution varies between 32 bit and 64 bit architectures. It the present case we may just want to filter the output a bit. |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM modulo a minor nit.
At first I thought the Printexc
would fail under bytecode due to the dreaded bytecode_complete
but I then realized it will work fine, as you catch the exception without needing the full backtrace.
The PR now installs a handler on every cmd
interpreted in a central hot path of both Lin
and STM
. This can potentially affect our ability to trigger parallel or concurrency issues, so we should carefully review the CI logs.
Did the first Lin
version also do so? (I may be misremembering)
lib/util.mli
Outdated
@@ -41,6 +41,12 @@ val print_triple_vertical : | |||
val protect : ('a -> 'b) -> 'a -> ('b, exn) result | |||
(** [protect f] turns an [exception] throwing function into a [result] returning function. *) | |||
|
|||
val tag_exn_with : ('a -> string) -> ('a -> 'b) -> 'a -> 'b | |||
(** [tag_exn_with show run x] behaves as [run x] unless raises an |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: "unless it raises an"
It did, but it was inlined at the site of the call. I don’t know how keen the compiler is to inline across modules but the function is modeled after I fixed your small suggestion, also as a way to trigger another round of testing. |
Proposition of an alternate design for that functionality: maybe we could install the catch-all exception handler only at the very start of the execution of the whole sequence of commands on each domain, and using domain-local storage to store the currently running command. I’m not sure we have a similar safe storage mechanism for systhreads though 😅 |
Add the utilitary exception Uncaught_exception and function wrap_uncaught_exn to improve the error reports in cases of uncaught exceptions This will allow STM and Lin to display the command along with the exception when a command raises an exception but its description did not announce it so
The standard QCheck mechanism to catch exceptions that are raised in tests do not give much clue about the source (ie Lin command) of that exception This packs the unsoundly-specified command with the exception in such a case so that it can be displayed Before: ``` exception Failure("unexpected") ``` After: ``` exception Failure("unexpected") raised but not caught while running always_fail t ```
The standard QCheck mechanism to catch exceptions that are raised in tests do not give much clue about the source (ie STM command) of that exception This packs the unsoundly-specified command with the exception in such a case so that it can be displayed Before: ``` exception Failure("unexpected") ``` After: ``` exception Failure("unexpected") raised but not caught while running AlwaysFail () ```
I just updated this PR with the latest proposal: using an inline |
The standard QCheck mechanism to catch exceptions that are raised in tests do not give much clues about the source (ie Lin command) of that exception
So this creates a specific Unexpected_exception that will pack the command along with the original exception so that it can be explained when the tests errors out
Before:
After: