Prevent NPE by checking SentryTracer.timer for null again inside synchronized #2200
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2200 +/- ##
============================================
- Coverage 80.91% 80.90% -0.01%
Complexity 3313 3313
============================================
Files 236 236
Lines 12154 12155 +1
Branches 1615 1616 +1
============================================
Hits 9834 9834
Misses 1725 1725
- Partials 595 596 +1
Continue to review full report at Codecov.
|
| @@ -344,8 +344,10 @@ public void finish(@Nullable SpanStatus status) { | |||
|
|
|||
| if (timer != null) { | |||
| synchronized (timerLock) { | |||
| timer.cancel(); | |||
| timer = null; | |||
| if (timer != null) { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Double check lock, in order to not lock if timer is already null. Looks like a good pattern to me.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
📜 Description
💡 Motivation and Context
NPE caused by
timerbecomingnullafter acquiring thesynchronizedlock. Now we check again after acquiring the lock to ensure this doesn't happen.Fixes #2196
💚 How did you test it?
📝 Checklist
🔮 Next steps