Еще одна нить, идущая внутри мьютекса

Question

Я не понимаю, что не так с этим кодом. Иногда два потока начинают выполнять блок try. Я создаю новый экземпляр popo каждый раз, когда вызываю функцию. Может кто-нибудь разобраться в чем проблема?

public class Instance {
  private static AtomicInteger i = new AtomicInteger(0);

  public synchronized void incrementInstance() {
    i.getAndIncrement();
  }

  public synchronized void decrementInstance() {
    i.getAndDecrement();
  }

  public synchronized int getInstances() {
    return i.get();
  }
}

public class popo {
  private static volatile MyMutex instanceMutex = new MyMutex();

  public void doSomething() {
    synchronized (instanceMutex) {
      final Instance no = new Instance();
      if (no.getInstances() > 0) {
         instanceMutex.wait();
      } else {
         no.incrementInstance();
      }
    }

    try {
     // do something
    } finally {
        synchronized (instanceMutex) {
          final Instance no = new Instance();
          if (no.Instances() > 0) {
            no.decrementInstance();
          }
          instanceMutex.notify();
        }
     }
  }
  private static class MyMutex {}
}

Answer 1

Проблема в том, что вы создаете новый экземпляр no каждый раз, когда вызываете doSomething().

synchronized (instanceMutex) {
  // each thread will make its own instance here
  final Instance no = new Instance();

Вместо этого вам нужно объявить Instance no глобально и статически, как MyMutex.

final, кстати, означает только то, что ссылка на no не может быть изменена (то есть вы не могли написать no = new Instance() в каком-то другом месте вашей программы).

static - ключевое слово, которое вы ищете - это означает, что каждый поток будет ссылаться только на один экземпляр Instance no.

Если это не поможет, если у вас более одного потока, ожидающего одновременно, возможно, вы выпускаете свои потоки одновременно. Чтобы предотвратить это, вы захотите вернуться назад и проверить no.getInstances() после того, как вы подождали, например:

synchronized (instanceMutex) {
    final Instance no = new Instance();
    while (no.getInstances() > 0) { // this check will need to be synchronized as well
        instanceMutex.wait();
    }
    // and only increment `no` once you've made a successful check.
    no.incrementInstance();
}

Мораль: условия гонки непростые!

Answer 2

Я думаю, что код пахнет повсюду :-) На самом деле, такие вещи, как создание новых экземпляров, которые не делают ничего, кроме доступа к статическому полю, приводят в замешательство (см. Предыдущие ответы).Итак, вот что я предлагаю:

1. Вам следует избегать использования wait / notify, если вы пишете новый код.Взгляните на параллельный пакет и Item69 в книге «Эффективная Java».
2. Исходя из того, что я понимаю из вашего кода, вам понадобится семафор типа:

static final Semaphore SEMAPHORE = new Semaphore(1);
...

SEMAPHORE.take(); // blocks, only one thread is allowed to proceed

try{
   //critical section
} finally {
    SEMAPHORE.release(); // never blocks, always within a finally block
}

Затем вы можете использовать метод getQueueLength (), чтобы узнать, сколько потоков ожидает, и заменить используемый вами AtomicInteger.См. http://docs.oracle.com/javase/6/docs/api/java/util/concurrent/Semaphore.html

1. Подумайте об использовании исполнителя.Затем, после того, как вы отправите задачу, вы можете ждать будущего.
2. Если вы все еще хотите использовать ожидание / уведомление, убедитесь, что весь код находится в синхронизированном блоке и используется стандартная идиома (Item69из эффективной Java):

synchronized(instanceMutex) {
    while(< condition does not hold >) {
        obj.wait();
    }

    // Perform required actions
}

Answer 3

Блок try не находится внутри синхронизированного блока. Операционная система может приостановить поток и возобновить другой в любой момент, если один поток только что вышел из синхронизированного блока и был приостановлен, другой мог логически выполнить блок try до или одновременно с первым потоком, завершившим синхронизированный блок.

Если для выполнения блока try требуется только один поток, попробуйте следующее:

synchronized (instanceMutex) {
  final Instance no = new Instance();
  if (no.getInstances() > 0) {
    instanceMutex.wait();
  } else {
    no.incrementInstance();
  }
  try {
   // do something
  } finally {
    if (no.Instances() > 0) {
      no.decementInstance();
    }
    instanceMutex.notify();
  }
}