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: SentryTransaction#finish should not clear another transaction from the scope #1278

Merged
merged 2 commits into from
Feb 19, 2021

Conversation

maciejwalkowiak
Copy link
Contributor

📜 Description

SentryTransaction#finish should not clear another transaction from the scope

💡 Motivation and Context

Fixes #1269

💚 How did you test it?

📝 Checklist

  • I reviewed the submitted code
  • I added tests to verify the changes
  • I updated the docs if needed
  • No breaking changes

Copy link
Contributor

@marandaneto marandaneto left a comment

Choose a reason for hiding this comment

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

LGTM
changelog though

@codecov-io
Copy link

Codecov Report

Merging #1278 (4360620) into main (e0e8be8) will increase coverage by 0.12%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #1278      +/-   ##
============================================
+ Coverage     75.04%   75.16%   +0.12%     
- Complexity     1752     1756       +4     
============================================
  Files           183      183              
  Lines          6159     6161       +2     
  Branches        610      611       +1     
============================================
+ Hits           4622     4631       +9     
+ Misses         1258     1252       -6     
+ Partials        279      278       -1     
Impacted Files Coverage Δ Complexity Δ
sentry/src/main/java/io/sentry/Hub.java 75.51% <100.00%> (+0.14%) 76.00 <0.00> (+1.00)
...src/main/java/io/sentry/transport/RateLimiter.java 77.35% <0.00%> (+1.88%) 27.00% <0.00%> (+1.00%)
...n/java/io/sentry/transport/AsyncHttpTransport.java 65.65% <0.00%> (+3.03%) 9.00% <0.00%> (ø%)
...in/java/io/sentry/transport/NoOpEnvelopeCache.java 66.66% <0.00%> (+16.66%) 4.00% <0.00%> (+1.00%)
.../java/io/sentry/transport/CurrentDateProvider.java 100.00% <0.00%> (+33.33%) 3.00% <0.00%> (+1.00%)

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 e0e8be8...4360620. Read the comment docs.

@maciejwalkowiak maciejwalkowiak merged commit 86e9d16 into main Feb 19, 2021
@maciejwalkowiak maciejwalkowiak deleted the gh-1269 branch February 19, 2021 14:44
Comment on lines +560 to +561
if (scope.getTransaction() == transaction) {
scope.clearTransaction();
Copy link
Member

Choose a reason for hiding this comment

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

On Android where the scope is shared this would race since in between another transaction could have been set.

Another approach would be:

scope.clear(transaction)
And internal conditional atomic reference swap (i.e: https://docs.microsoft.com/en-us/dotnet/api/system.threading.interlocked.compareexchange?view=net-5.0)

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.

SentryTransaction.finish() clears transaction from the Scope even if it was not bound to the Scope at all
4 participants