Skip to content

Conversation

@Swatinem
Copy link
Contributor

@Swatinem Swatinem commented Jun 29, 2020

This reworks the task locking and background worker shutdown logic.
Tasks are now refcounted so they can be executed concurrently while another thread removes them from the queue. In case the background worker can’t be cleanly shut down (due to slow network), the task queue is being dumped, but the background worker itself is not freed from the main thread, to avoid potential use-after-free bugs.
Also, the background worker itself is refcounted and owns its state.

The transport state is now owned by the background worker, and passed explicitly to the task send function. The task state itself was removed in favor of providing the envelope directly.

@Swatinem Swatinem requested a review from a team June 29, 2020 11:14
@codecov-commenter
Copy link

codecov-commenter commented Jun 29, 2020

Codecov Report

Merging #323 into master will increase coverage by 0.37%.
The diff coverage is 93.10%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #323      +/-   ##
==========================================
+ Coverage   87.18%   87.55%   +0.37%     
==========================================
  Files          49       49              
  Lines        3971     3993      +22     
==========================================
+ Hits         3462     3496      +34     
+ Misses        509      497      -12     
Impacted Files Coverage Δ
src/sentry_sync.h 97.36% <ø> (+0.07%) ⬆️
src/sentry_sync.c 93.33% <91.01%> (+2.89%) ⬆️
src/transports/sentry_transport_curl.c 91.59% <95.74%> (+2.25%) ⬆️
src/sentry_core.c 95.33% <100.00%> (+0.93%) ⬆️
src/sentry_logger.h 100.00% <0.00%> (ø)
src/sentry_utils.c 90.11% <0.00%> (+0.03%) ⬆️
src/sentry_value.c 89.81% <0.00%> (+0.18%) ⬆️
src/sentry_options.c 77.77% <0.00%> (+0.22%) ⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 577c9f4...87ee001. Read the comment docs.

@Swatinem Swatinem marked this pull request as ready for review June 30, 2020 08:37
@Swatinem Swatinem requested a review from jan-auer July 3, 2020 13:14
@jan-auer
Copy link
Member

jan-auer commented Jul 6, 2020

Notes from an offline discussion: When the crash happens close to startup, it is more likely that it interrupts an ongoing upload. In this case, it would be better to avoid dropping it.

If we refcount the envelopes in the transport queue, we could have multiple threads read and clean the queue concurrently.

@Swatinem Swatinem force-pushed the fix/cancel-threads branch from 2f50fa6 to 0073054 Compare July 8, 2020 11:20
Copy link
Member

@jan-auer jan-auer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good now. I'll to a final run through this and the tests tomorrow, to check if we're lacking coverage for anything vital.

Copy link
Member

@jan-auer jan-auer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good to go now. Thank you for taking a deep dive into this.

@Swatinem Swatinem merged commit 62c9810 into master Jul 9, 2020
@Swatinem Swatinem deleted the fix/cancel-threads branch July 9, 2020 10:58
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.

5 participants