synchronizedのバグパターンを1つ知った

隣の炎上プロジェクトのヤパイコード探しを仰せつかったので、とりあえずFindBugsにかけてみると、↓こんなのが出た。

class Hoge {
    private String foo;
    public void setFoo( String foo ) {
        synchronized( this.foo ) { // fooへの更新を1スレッドに制限したい(のだろうと推測)
            this.foo = foo;
            <処理...>
        }
    }
}

「this.foo」のロックで同期化をしようとしているのだけど、synchronizedブロック内でthis.fooを書き換えてしまっている。書き換えた後のthis.fooは誰もロックしていないため、ブロックが複数スレッドから実行できてしまう。以下は検証コード。

class Foo {
    private String foo = ""; // nullを設定されるとエラーになる、とかいう問題もあるなー。
    public void setFoo( String foo ) {
        synchronized( this.foo ) { // fooへの更新を1スレッドに制限したい(のだろうと推測)
            this.foo = foo;
            
            // わかりやすいように処理を追加。
            // this.foo へのアクセスは同期化されているので以下は順番に実行されるはず。
            for ( int i = 0 ; i < 3; i++ ) {
                System.out.println(i);
            }
        }
    }
}

5多重で、setFoo()を実行。

final Foo foo = new Foo();

Thread[] ts = new Thread[5];
for ( int i = 0; i < ts.length; i++ ) {
    final String str = String.valueOf(i);
    ts[i] = new Thread() {
        public void run() {
            foo.setFoo(str);
        }
    };
}
for ( Thread t : ts ) {
    t.start();
}
for ( Thread t : ts ) {
    t.join();
}

実行結果です。synchronizedブロックが並列で実行されています。

対策

で、対策だけど、こういうときは、private final な専用のオブジェクトを使うのが常套だよね!

class Foo2 {
    private final Object monitor = new Object(); // 専用のロック用オブジェクト
    private String foo = ""; 
    public void setFoo( String foo ) {
        
        synchronized( monitor ) { // this.fooではなくmonitorをロック。
            this.foo = foo;
            
            for ( int i = 0 ; i < 3; i++ ) {
                System.out.println(i);
            }
        }
    }
}

実行結果です。ちゃんと同期化されてます。