嵌套的同步块是必要的吗？

Question

我正在写一个多线程程序，我有一个具有嵌套的同步块的方法，我想知道我是否需要内部同步，或者只需要外部同步就足够了。

public class Tester {

    private BlockingQueue<Ticket> q = new LinkedBlockingQueue<>();
    private ArrayList<Long> list = new ArrayList<>();

    public void acceptTicket(Ticket p) {
        try {
            synchronized (q) {
                q.put(p);

                synchronized (list) {
                    if (list.size() < 5) {
                        list.add(p.getSize());
                    } else {
                        list.remove(0);
                        list.add(p.getSize());
                        }
                }
            }
        } catch (InterruptedException ex) {
            Logger.getLogger(Consumer.class.getName()).log(Level.SEVERE, null, ex);
        }

    }
}

编辑:这不是一个完整的类，因为我还在开发它。但从本质上讲，我是在模仿售票机。售票机在BlockingQueue q中维护票证列表。每当客户端向机器添加票证时，售票机还会跟踪最近5张票证的价格(ArrayList列表)。因此，我认为我不需要内部同步，因为只有这个类(票证系统)才能访问list

Answer 1

危险之处在于：

    synchronized (q) {
        synchronized (list) {
         }
    }

总有一天你会添加一个方法，或者以这样的顺序调用方法，实际上它也是这样做的：

    synchronized (list) {
        synchronized (q) {
         }
    }

那它就是一个死锁的定时炸弹。

由于这是一个完整的类，你可以只锁定一个，或者像平常一样，在方法上使用synchronized来锁定对象本身。对这些私有的所有其他访问也需要同步。

public class Tester {

private BlockingQueue<Ticket> q = new LinkedBlockingQueue<>();
private ArrayList<Long> list = new ArrayList<>();

public synchronized void acceptTicket(Ticket p) {
    try {
      q.put(p);

      if (list.size() < 5) {
        list.add(p.getSize());
      } else {
        list.remove(0);
        list.add(p.getSize());
      }
    } catch (InterruptedException ex) {
        Logger.getLogger(Consumer.class.getName()).log(Level.SEVERE, null, ex);
    }    
}

或更干净：

public class Tester {

private BlockingQueue<Ticket> q = new LinkedBlockingQueue<>();
private ArrayList<Long> list = new ArrayList<>();

public void acceptTicket(Ticket p) {
    try {
      //this is cleaner, because I don't know what logger class is doing,
      //I want to eliminate chance of deadlock and reduce time we are in lock
      synchronized (this){
        q.put(p);

        if (list.size() < 5) {
          list.add(p.getSize());
        } else {
          list.remove(0);
          list.add(p.getSize());
        }
     }
    } catch (InterruptedException ex) {
        Logger.getLogger(Consumer.class.getName()).log(Level.SEVERE, null, ex);
    }    
}

更干净，但在你的情况下不太可能是必要的(根据我所看到的)：

public class Tester {

private final Object lockObj = new Object(); //specific object for locking
                                //could use any other private, non-exposed final but
                                //this makes it absolutely clear what I should be
                                //using for locks
private BlockingQueue<Ticket> q = new LinkedBlockingQueue<>();
private ArrayList<Long> list = new ArrayList<>();

public void acceptTicket(Ticket p) {
    try {
      //"this" can be locked by external code outside my control here,
      //so I use a specific private final object lockObj to eliminate deadlocks and
      //provide finer grained locking - reducing contension
      synchronized (lockObj){
        q.put(p);

        if (list.size() < 5) {
          list.add(p.getSize());
        } else {
          list.remove(0);
          list.add(p.getSize());
        }
     }
    } catch (InterruptedException ex) {
        Logger.getLogger(Consumer.class.getName()).log(Level.SEVERE, null, ex);
    }    
}

Answer 2

看起来上面代码的目的是将所有操作(队列中的存储和列表中的存储)分组到一个原子操作中。所以，是的，外部的同步块就是您所需要的。一次持有多个锁是导致死锁的好方法，因此应尽可能避免这种情况。

不过，我不会使用队列本身作为锁，而是使用一个专用的最终锁对象。

最重要的是:上面的代码本身没有任何意义，因为列表和队列不会在其他地方使用。因此，我刚才所说的可能是真的，也可能是完全错误的，这取决于其余的实际代码。