Skip to content
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

docs: clarify behavior of sentry_flush + sentry_close #883

Merged
merged 5 commits into from
Sep 13, 2023

Conversation

sapphonie
Copy link
Contributor

@sapphonie sapphonie commented Sep 12, 2023

This PR

  • clarifies that sentry_shutdown does not remove any crash handlers

Copying over a conversation from the Sentry discord;

supervacuus — Today at 4:25 AM
@sappho
The crashpad client does not provide a mechanism to uninstall its in-process crash-handler nor shutdown the crashpad_handler executable. So the handler is still installed after sentry_close().

On Linux we brutally restore signal handlers without making crashpad aware of this. Maybe we should do the same on Windows?

If you want to disable the crash-handler you can call SetUnhandledExceptionFilter(NULL) which will disable the local handler installed by crashpad. This can happen in your Shutdown() method right after invoking sentry_close().

More TODO involving

  • documenting if sentry_flush() is async or not done, it is not async, it blocks
  • documenting if sentry_close() is async or not done, see above
  • documenting in example.c that crash handling will still be called after sentry_shutdown()

#skip-changelog

@codecov
Copy link

codecov bot commented Sep 12, 2023

Codecov Report

Merging #883 (abfcb19) into master (d9e9019) will increase coverage by 6.24%.
The diff coverage is n/a.

❗ Current head abfcb19 differs from pull request most recent head 8d9115a. Consider uploading reports for the commit 8d9115a to get more accurate results

@@            Coverage Diff             @@
##           master     #883      +/-   ##
==========================================
+ Coverage   76.32%   82.56%   +6.24%     
==========================================
  Files          53       53              
  Lines        7366     7366              
  Branches     1185     1185              
==========================================
+ Hits         5622     6082     +460     
+ Misses       1195     1175      -20     
+ Partials      549      109     -440     

@supervacuus
Copy link
Collaborator

Thanks @sapphonie! Could you let me know if you're done with this PR? Is it okay if I take over and handle a few technicalities?

#skip-changelog

@sapphonie
Copy link
Contributor Author

Thanks @sapphonie! Could you let me know if you're done with this PR? Is it okay if I take over and handle a few technicalities?

#skip-changelog

Sure, go ahead

@sapphonie sapphonie marked this pull request as ready for review September 12, 2023 12:59
@supervacuus supervacuus changed the title clarify documentation regarding sentry_shutdown and others docs: clarify behavior of sentry_flush + sentry_close Sep 13, 2023
@supervacuus supervacuus merged commit b2c2ee0 into getsentry:master Sep 13, 2023
18 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants