乱同步……

昨天在用findbugs扫我们的代码时看到了类似这样的bug提示：（嗯……真的方法名忽略吧）

findbugs 写道

Bug: alpha.beta.charlie.Foo.bar(String) calls Thread.sleep() with a lock held
Pattern id: SWL_SLEEP_WITH_LOCK_HELD, type: SWL, category: MT_CORRECTNESS

This method calls Thread.sleep() with a lock held. This may result in very poor performance and scalability, or a deadlock, since other threads may be waiting to acquire the lock. It is a much better idea to call wait() on the lock, which releases the lock and allows other threads to run.

那段代码看起来与下面的Foo.bar()类似：

public class TestSync {
  public static void main(String[] args) throws Exception {
    Foo foo = new Foo();
    Thread t1 = new Thread(new MyRunnable(foo));
    Thread t2 = new Thread(new MyRunnable(foo));
    t1.start();
    t2.start();
  }
}

class MyRunnable implements Runnable {
  private Foo foo;
  
  public MyRunnable(Foo foo) {
    this.foo = foo;
  }
  
  public void run() {
    foo.bar("alpha");
  }
}

class Foo {
  public void bar(String name) {
    String key = name + "_"; // this will create a new "key" object every time invoked
    synchronized (key) {
      try {
        System.out.println("1: " + name);
        Thread.sleep(1000);
        System.out.println("2: " + name);
        Thread.sleep(5000);
      } catch (InterruptedException e) {
        System.err.println("interrupted");
      }
    }
  }
}

可以看到输出经常是：

1: alpha
1: alpha
2: alpha
2: alpha

嗯，在synchronized块里用了Thread.sleep()，是很邪恶。正准备看看有没有什么办法去修一下，突然发现更糟糕的问题是那synchronized块完全是废的：每次Foo.bar()被调用的时候，“key”都是一个新生成的String对象；于是锁住的根本不是同一个对象，实际上没达到锁的目的。

如果把“+”去掉，并且确保输入的参数是指向同一个String对象（Java里字符串字面量由VM保证会被intern），再看程序的行为就很不同了：

public class TestSync {
  public static void main(String[] args) throws Exception {
    Foo foo = new Foo();
    Thread t1 = new Thread(new MyRunnable(foo));
    Thread t2 = new Thread(new MyRunnable(foo));
    t1.start();
    t2.start();
  }
}

class MyRunnable implements Runnable {
  private Foo foo;
  
  public MyRunnable(Foo foo) {
    this.foo = foo;
  }
  
  public void run() {
    foo.bar("alpha");
  }
}

class Foo {
  public void bar(String name) {
    String key = name; // notice this doesn't create a new object
    synchronized (key) {
      try {
        System.out.println("1: " + name);
        Thread.sleep(1000);
        System.out.println("2: " + name);
        Thread.sleep(5000);
      } catch (InterruptedException e) {
        System.err.println("interrupted");
      }
    }
  }
}

输出总是：

1: alpha
2: alpha
1: alpha
2: alpha

然而String这东西，普遍来说在运行时即便equals也无法保证是“同一对象”，所以这代码本身的设计就很有问题，不光是在Thread.sleep()上了。

诶，生产力就消磨在起JBoss，看错误log，关JBoss刷Maven，改代码，刷Maven来build然后再起JBoss……而且遗留下来的问题各种诡异。findbugs默认抓的虫也还不够多 =_=|||| =o) TvT||||

评论

3 楼 liaofeng_xiao 2010-04-03  

嗯，同步锁对象必须是公共对象，否则就好像每个人都有一把锁的钥匙，那这把锁也就没有存在实际意义了。。。

2 楼 RednaxelaFX 2009-12-30  

icewubin 写道

建议用读写锁，全控制。

诶其实我遇到的那段代码根本不用锁，也不用Thread.sleep()...纯粹是原本的代码逻辑有问题而已。

1 楼 icewubin 2009-12-30  

建议用读写锁，全控制。