nisiyong commented on a change in pull request #6009:
URL: https://github.com/apache/skywalking/pull/6009#discussion_r543115337
##########
File path:
apm-commons/apm-datacarrier/src/main/java/org/apache/skywalking/apm/commons/datacarrier/consumer/MultipleChannelsConsumer.java
##########
@@ -30,13 +32,12 @@
public class MultipleChannelsConsumer extends Thread {
private volatile boolean running;
private volatile ArrayList<Group> consumeTargets;
- @SuppressWarnings("NonAtomicVolatileUpdate")
- private volatile long size;
+ private final AtomicLong size = new AtomicLong();
Review 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 `volatile` keyword guarantees visibility only!
If you don't want to use AtomicLong, I am willing to just add `synchronized`
on the `MultipleChannelsConsumer#addNewTarget` method.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
