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.

Sign up for GitHub

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

Jump to bottom

Prevent NPE by checking SentryTracer.timer for null again inside synchronized #2200

Merged
merged 2 commits into from
Aug 3, 2022
Merged

Prevent NPE by checking SentryTracer.timer for null again inside synchronized #2200

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b7797f9
Prevent NPE by checking SentryTracer.timer for null again inside sync…
adinauer Aug 3, 2022
7cab097
Add changelog
adinauer Aug 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

### Fixes

- Prevent NPE by checking SentryTracer.timer for null again inside synchronized ([#2200](https://github.com/getsentry/sentry-java/pull/2200))
- `attach-screenshot` set on Manual init. didn't work ([#2186](https://github.com/getsentry/sentry-java/pull/2186))
- Remove extra space from `spring.factories` causing issues in old versions of Spring Boot ([#2181](https://github.com/getsentry/sentry-java/pull/2181))

Expand Down
10 changes: 6 additions & 4 deletions sentry/src/main/java/io/sentry/SentryTracer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,8 @@ public final class SentryTracer implements ITransaction {
*/
private final @Nullable Long idleTimeout;

private @Nullable TimerTask timerTask;
private @Nullable Timer timer = null;
private volatile @Nullable TimerTask timerTask;
private volatile @Nullable Timer timer = null;
private final @NotNull Object timerLock = new Object();
private final @NotNull SpanByTimestampComparator spanByTimestampComparator =
new SpanByTimestampComparator();
Expand Down Expand Up @@ -344,8 +344,10 @@ public void finish(@Nullable SpanStatus status) {

if (timer != null) {
synchronized (timerLock) {
timer.cancel();
timer = null;
if (timer != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this make the L345 redundant? Since every other operation is guarded by timerLock.
The benefit is that you spare the synchronized contention if timer is null, so makes sense to keep it.

Please do the same for LifecycleWatcher.

Copy link

Choose a reason for hiding this comment

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

Double check lock, in order to not lock if timer is already null. Looks like a good pattern to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, as @brustolin said, it's there to avoid the lock if it isn't needed.

@marandaneto created #2202 to track the double checked locking improvement.

Copy link
Contributor

Choose a reason for hiding this comment

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

The double lock is a bad pattern in Java and is broken by default @brustolin
In this case, it works, since you made the field volatile as well, and the first lock is just for the avoidance of contention

Copy link
Member

Choose a reason for hiding this comment

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

That seems specific to the JVM. Does it affect ART? Also, there are some 'theoreticals' in there (I just skimmed though)
Also, we're not initializing the Timer in this method, so what he's talking about doesn't seem to apply here.

Copy link
Contributor

Choose a reason for hiding this comment

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

The shared article is more about how you can do the double-checked locking wrong, and that's why it's considered an anti-pattern, the example indeed isn't the same, but I shared it just for context since @brustolin mentioned "good pattern".

In this case, it works, since you made the field volatile as well

Copy link
Contributor

Choose a reason for hiding this comment

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

https://stackoverflow.com/questions/5717090/double-checked-locking-in-android

timer.cancel();
timer = null;
}
}
}

Expand Down