[hyddd的FindBugs分析记录][M M IS] Inconsistent synchronization

[M M IS] Inconsistent synchronization [IS2_INCONSISTENT_SYNC]

The fields of this class appear to be accessed inconsistently with respect to synchronization.  This bug report indicates that the bug pattern detector judged that

1. The class contains a mix of locked and unlocked accesses,
2. At least one locked access was performed by one of the class's own methods, and
3. The number of unsynchronized field accesses (reads and writes) was no more than one third of all accesses, with writes being weighed twice as high as reads

A typical bug matching this bug pattern is forgetting to synchronize one of the methods in a class that is intended to be thread-safe.

You can select the nodes labeled "Unsynchronized access" to show the code locations where the detector believed that a field was accessed without synchronization.

Note that there are various sources of inaccuracy in this detector; for example, the detector cannot statically detect all situations in which a lock is held.  Also, even when the detector is accurate in distinguishing locked vs. unlocked accesses, the code in question may still be correct.

 

先上一段代码：

public class Test  extends Thread{

    public static void main(String args[]) throws InterruptedException{
        ObjectClass obj = new ObjectClass(); 
   
        Thread t2 = new ChangeValue(obj);
        t2.start();
           
        Thread t1 = new AlwaysRun(obj);
        t1.start();

        sleep(10000);
        t1.stop();
    }
}

class AlwaysRun extends Thread{
    
    ObjectClass obj;
    
    public AlwaysRun(ObjectClass obj) {
        // TODO Auto-generated constructor stub
        this.obj = obj;
    }
    
    public void run() {
        obj.Loop();
    }
}

class ChangeValue extends Thread{
    
    ObjectClass obj;
    
    ChangeValue(ObjectClass obj){
        this.obj = obj;
    }
    
    public void run() {

        System.out.println("Thread2");
        ObjClass2 obj2 = obj.getObj();
        
        try {
            sleep(1500);
        } catch (InterruptedException e) {
            System.out.println("Error!");
        }
        
               obj2.str = "aaa";
        System.out.println("Thread2 Finish!");
    }
}

public class ObjectClass extends Thread {

    private ObjClass2 obj;
    private Object lockTable = new Object();
    
    public ObjectClass() {
        // TODO Auto-generated constructor stub
        obj = new ObjClass2();
    }
    
    public void setObj(ObjClass2 obj){
        synchronized (lockTable){
            this.obj = obj;      
        }
    }
    
    public ObjClass2 getObj(){
        synchronized (lockTable){
            return this.obj;      //出问题处！！
        }
    }
    
    public void Loop(){
        synchronized (lockTable){
            while(true){
                System.out.println(obj.str);
            }
        }
    }    
}

public class ObjClass2 {    
    public String str = "ddddddd";    
}

看看运行的结果：

Thread2
ddddddd
ddddddd
ddddddd
ddddddd
ddddddd
ddddddd
ddddddd
Thread2 Finish!
aaa
aaa
aaa
aaa
aaa
aaa
....

如果看明白代码，你应该会知道问题出再哪里了，为什么Loop使用了synchronized (lockTable)，但是obj.str还是被修改了？！因为getObj()这个函数把obj对象返回了给外面，JAVA里面对象的传递是使用引用传递，如果对象传递到外面并且在做修改obj的时候没有加锁操作，就是引起刚才的问题。所以如果getObj()函数返回的是对象，那么，请返回一个拷贝，而不要直接返回引用。

这里再总结一下值得注意问题：

1.看下面代码：

    public ObjClass2 getObj(){
        synchronized (lockTable){
            return this.obj;
        }
    }

synchronized (lockTable)不能阻止外面的函数修改obj，即：obj=getObj();当赋值完毕后，synchronized (lockTable)无效了，如果后面需要修改obj的值，那么就得注意了！！！

另外建议的是，不直接返回this.obj，而是返回一个this.obj的拷贝。这样可以根本上避免出现上面的问题！

2.同理，在setObj(...)的时候，如果传入的是个对象，也建议是存储传入对象的拷贝，而不（this.obj=obj）这样直接赋值。

3.注意对竞争的资源都使用synchronized (lockTable)，不要像上面的Demo代码那样，一处用了，一处没有！