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

fix: Lock global options whenever they are accessed #333

Merged
merged 7 commits into from
Jul 23, 2020

Conversation

Swatinem
Copy link
Member

@Swatinem Swatinem commented Jul 8, 2020

Wraps all accesses of the global objects object in a lock. This also changes the DSN to be a heap allocated and refcounted type, since the http workers need to separately own a reference.

fixes #280

@codecov-commenter
Copy link

codecov-commenter commented Jul 8, 2020

Codecov Report

Merging #333 into master will increase coverage by 0.29%.
The diff coverage is 78.98%.

@@            Coverage Diff             @@
##           master     #333      +/-   ##
==========================================
+ Coverage   87.37%   87.66%   +0.29%     
==========================================
  Files          49       49              
  Lines        4039     4045       +6     
==========================================
+ Hits         3529     3546      +17     
+ Misses        510      499      -11     

@Swatinem Swatinem force-pushed the fix/lock-options branch 2 times, most recently from 644ba11 to bdc0060 Compare July 9, 2020 11:00
@Swatinem Swatinem requested a review from a team July 9, 2020 11:09
@Swatinem Swatinem marked this pull request as ready for review July 9, 2020 11:09
src/sentry_utils.c Outdated Show resolved Hide resolved
flub
flub previously approved these changes Jul 10, 2020
tests/test_integration_http.py Show resolved Hide resolved
src/backends/sentry_backend_breakpad.cpp Outdated Show resolved Hide resolved
src/backends/sentry_backend_breakpad.cpp Outdated Show resolved Hide resolved
src/sentry_core.c Outdated Show resolved Hide resolved
src/sentry_core.c Outdated Show resolved Hide resolved
src/sentry_core.c Outdated Show resolved Hide resolved
src/sentry_core.c Outdated Show resolved Hide resolved
src/transports/sentry_transport_curl.c Outdated Show resolved Hide resolved
This also changes the DSN to a heap allocated, refcounted type since
the http worker thread needs to hold onto its own reference.
this now decouples preparing an event, which also removes the need for a special breakpad transport and mutating the global transport. Also the crashed session is now attached to the crashed envelope, and has a correct errors count

options are now refcounted internally instead of taking locks for long times. the user consent is now done as an atomic operation.
@Swatinem Swatinem dismissed flub’s stale review July 22, 2020 14:01

reworked everything

@Swatinem
Copy link
Member Author

After discussing this a little bit, I completely reworked this again.
Instead of holding the lock for a long period of time, the locked section is now super small, just loading the global options and incrementing their refcount.
the SENTRY_WITH_OPTIONS macro now does that and decrements the refcount afterwards.

The only mutable access to options now is the user consent, which was changed to an atomic.

Instead of swapping out transports, capturing an event was split up into preparing the event, and then capturing it with an explicit transport. This also gets rid of the special case breakpad transport because the minidump can now be attached directly to an envelope instead of making roundtrips via that special transport.

While at it, inproc/breakpad now only send a single envelope on crash, with the session in it, which also now has an error count that covers the crash itself.

include/sentry.h Outdated Show resolved Hide resolved
include/sentry.h Outdated Show resolved Hide resolved
include/sentry.h Outdated Show resolved Hide resolved
include/sentry.h Outdated Show resolved Hide resolved
src/sentry_core.c Outdated Show resolved Hide resolved
@Swatinem Swatinem merged commit b9e2aab into master Jul 23, 2020
@Swatinem Swatinem deleted the fix/lock-options branch July 23, 2020 12:38
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.

sentry_shutdown can break things in a multi-threaded environment.
4 participants