-
-
Notifications
You must be signed in to change notification settings - Fork 165
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
ref(on_crash): follow-up to make on_crash releasable #734
Conversation
* mentioned in the header docs `before_send` vs `on_crash` behavior * adapted `sentry__prepare_event` to provide a flag for backends on whether `before_send` should be invoked * adapted backends to prevent running `before_send` even if `on_crash` returns true * added `on_crash` vs `before_send` integration tests for `inproc` and `breakpad` (`crashpad` will follow) * removed `stdbool.h` from `sentry.h` and replaced return value `bool` with `int` in `sentry_crash_function_t` (since bool seemed to be explicitly prevented in the header up to now) * added remaining CHANGELOG entries for upcoming release
@@ -2,12 +2,25 @@ | |||
|
|||
## Unreleased |
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.
0.5.0 or 0.4.19? Moving the sampling to the end of the pre-processing (in #729), thereby executing before_send()
unconditionally, could be considered a breaking change. I am fine either way.
// * we need integration tests for more signal/exception types not only | ||
// for unmapped memory access (which is the current crash in example.c). | ||
// * we should adapt the SetFirstChanceExceptionHandler interface in | ||
// crashpad |
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.
Another option is, as mentioned above, to change the state of the global signal configuration (aka reset to default handlers... which will in turn terminate the app). This looks cleaner on the surface, but I am not sure how happy crashpad is if we pull the rug in the first-chance handler. This is different from resetting when shutting down.
} else if (options->before_send_func) { | ||
sentry_value_t event = sentry_value_new_event(); | ||
SENTRY_TRACE("invoking `before_send` hook"); | ||
event = options->before_send_func( | ||
event, nullptr, options->before_send_data); | ||
should_dump = !sentry_value_is_null(event); |
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.
This was missing up to now. For the crashpad backend, it was irrelevant whether you returned the event or a null value.
The other difference (which I have not changed) to the other backends: we create the event right before the hook and then discard it again. The other backends allow changes made to the event received in before_send()
to land in the backend.
sentry_envelope_t *envelope | ||
= sentry__prepare_event(options, event, NULL); | ||
sentry_envelope_t *envelope = sentry__prepare_event( | ||
options, event, nullptr, !options->on_crash_func); |
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.
without informing sentry__prepare_event()
whether before_send()
should be invoked, we cannot make sure that a set on_crash()
prevents the execution of the before_send()
callback.
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.
well its almost a bit late, but how about we make the on_crash
take and return a sentry_value_t
, similar to before_send
.
At least for the inproc
backend, we actually use that very event. The other backends discard it anyway and take all the info from the minidump.
But for in-proc, it might still be useful to add custom stuff to the event in case of hard crash.
// (currently returning `true`) | ||
// | ||
// TODO(supervacuus): | ||
// * we need integration tests for more signal/exception types not only |
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.
We have assert
/ abort
in there, do these cover this?
I think this would be a much better interface and more future-proof. I think the best time to make such a change is right now before the interface is released (even considering it is experimental). |
before_send
vson_crash
behaviorsentry__prepare_event
to provide a flag for backends onwhether
before_send
should be invokedbefore_send
even ifon_crash
returns true
on_crash
vsbefore_send
integration tests forinproc
andbreakpad
(crashpad
will follow)stdbool.h
fromsentry.h
and replaced return valuebool
with
int
insentry_crash_function_t
(sincebool
seemed to beexplicitly prevented in the header up to now)