fix: Non-atomic operation on volatile field #6009
Conversation
Codecov Report
@@ Coverage Diff @@
## master #6009 +/- ##
============================================
- Coverage 51.26% 49.20% -2.06%
+ Complexity 3513 3357 -156
============================================
Files 1680 1680
Lines 35749 35772 +23
Branches 3949 3937 -12
============================================
- Hits 18325 17601 -724
- Misses 16493 17235 +742
- Partials 931 936 +5 Continue to review full report at Codecov.
|
| private volatile ArrayList<Group> consumeTargets; | ||
| @SuppressWarnings("NonAtomicVolatileUpdate") | ||
| private volatile long size; | ||
| private final AtomicLong size = new AtomicLong(); |
There was a problem hiding this comment.
Why need atomic long? We don't do concurrency consuming in any case
There was a problem hiding this comment.
Yes, there has another way to fix this warning. Just add synchronized on the MultipleChannelsConsumer#addNewTarget, but it has already added on BulkConsumePool#add which invoke MultipleChannelsConsumer#addNewTarget.
IDE shows warning because the volatile filed has non-atomic operations on this Class, the non-atomic operations should be synchronized. Or we just use AtomicLong.
There was a problem hiding this comment.
I don't find the real case, the reason for warning is this could happen, but in our case, there isn't.
There was a problem hiding this comment.
It doesn't mean there has a wrong logic in the release code, maybe it runs well. But I just want to indicate that was a bad practice to use volatile in this way.
If there are multi-threads invoke the MultipleChannelsConsumer#addNewTarget method and the field size still uses volatile, the size maybe a wrong value. Because
The Java
volatilekeyword guarantees visibility only!
If you don't want to use AtomicLong, I am willing to just add synchronized on the MultipleChannelsConsumer#addNewTarget method.
There was a problem hiding this comment.
We are trying to avoid any possible lock, it is also slower, more or less. That is why it may be not a good idea.
SkyWalking codes are for product environment and high performance, we have no plan to use the codes to guide people how to write codes.
So, the key is, if the current version of codes has a bug, we are open to the solution.
There was a problem hiding this comment.
Okay, I can revert MultipleChannelsConsumer first.
But the SimpleRollingPartitioner has a bug, and I have added a test case. Please take a look.
| @SuppressWarnings("NonAtomicVolatileUpdate") | ||
| private volatile int i = 0; | ||
|
|
||
| private final AtomicInteger i = new AtomicInteger(); |
There was a problem hiding this comment.
Let's talk about this. I know your point of bug, it would not increase every time. But it has higher performance with no lock, in the partition, there is a lock to avoid the real issue. So this is actually not a bug in the real world.
This is another typical thing, codes are not by the book.
There was a problem hiding this comment.
in the partition, there is a lock to avoid the real issue.
Could you tell me where the lock is? I want to take a look.
There was a problem hiding this comment.
Channel index. There is a atomic integer as index pointer.
wu-sheng
added
invalid
2 participants
Fix Non-atomic operation on volatile field
CHANGESlog.Recently I dive into SkyWalking sources code, and I find some bugs in the
DataCarriermodule.It runs with something wrong although they won't affect the main feature.
IntelliJ IDEA shows the warning as follow: