-
-
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
feat: invoke on_crash when handling a crash #724
Conversation
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 looks very good!
Supporting the uctx on breakpad linux / macos would be nice.
It would also be interesting to highlight the difference between this and before_send
.
Currently, before_send
in some cases gets an empty event that is being discarded, just to satisfy its function signature, and to give users the ability to return a null
event, in which case no crash report is being sent.
We should do something similar, maybe add a boolean return value to the hook, to indicate if we would capture the crash, or ignore it.
Another interesting aspect would be to maybe phase out before_send
as currently used in the crash handler, meaning:
- If
on_crash
is defined,before_send
is only invoked for normal events, but not for crashes. - If
on_crash
is not defined, we need to maintain the currentbefore_send
behavior for backwards compatibility.
This would also give users the possibility to opt into the new behavior just by providing an empty noop on_crash
handler.
As far as I can see, in case of crash report return value is ignored, so there is no option to discard the event: sentry-native/src/backends/sentry_backend_crashpad.cpp Lines 140 to 146 in b091b76
I'm not sure if it make sense to change this behaviour since you want to preserve backwards compatibility. UPD: |
a69b7c8
to
cfa744f
Compare
Which I would say is a bug then that we need to fix, I vaguely remember customers asking about this. A dedicated Though it should be well documented in which permutations we have what kind of guarantees. As in: crashpad on macOS would not support any of that :-( |
Adjusted as discussed. readme is not updated yet. |
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.
One thing to note here, and I think this is very much in line with the recent changes to sessions, more specifically, we shouldn’t touch them when the event is being discarded by hooks. But that also means that sessions would be marked as "abnormal" instead of "crashed", but I think this is expected. CC @adinauer
It looks like our CI is broken right now, but I will merge this as soon as we fix that. |
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.
Sorry for this last minute change request ;-)
Also we fixed our CI, so please merge/rebase master.
include/sentry.h
Outdated
* The callback passes a pointer to sentry_ucontext_s structure when exception | ||
* handler is invoked. For breakpad on Linux this pointer is NULL. | ||
* | ||
* If the callback returns true outgoing crash report will be discarded. |
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 might just be personal preference, but I think its also more consistent with similar callbacks:
Can you turn this logic around? So that false
discards the event, and true
will continue processing as normal?
This is also in line with how before_send
behaves, as you return a null
value to discard the 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.
I already inverted it to be consistent with crashpad and breakpad handler returns
But it makes sense, especially since it's not used for breakpad return value anymore
Though, maybe it also shouldn't be used for crashpad as well there?
sentry-native/src/backends/sentry_backend_crashpad.cpp
Lines 188 to 190 in e8011b6
// further handling can be skipped via on_crash hook | |
return already_handled; |
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.
I admit this is a bit confusing. Ideally the SDK users should not care about the underlying handler, and we can return an appropriate value to make the handler behave like it should.
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.
The problem is I'm not sure what is the desired behaviour.
The docs says
If the custom handler returns true, the signal is considered handled and the signal handler returns. Otherwise, the currently installed Crashpad signal handler is run.
But the actual behaviour should depend on some formal requirements
I reverted the return to false
for crashpad too. Let me know if you think it should depend on on_crash
return
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.
I reverted the return to false for crashpad too. Let me know if you think it should depend on on_crash return
Yes I think it should. Essentially we need to return !should_handle
, as returning true
from the crashpad hook tells it to not handle (aka send to sentry).
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.
alright, should be fine now
- don't invoke before_send if on_crash is set - if on_crash returns false crash report will be discarded
for #592
not sure if it's possible to add some UT for that