Вероятный ложный положительный результат от Findbugs?

Пожалуйста, обратитесь к следующему фрагменту кода (убрана лишняя часть, чтобы выделить проблемный случай):

FindBugs жалуется, что "Метод не снимает блокировку на всех путях". Это ложное срабатывание? Если нет, то как это исправить?

  try{
      someLock.lock();
     //do something
    } finally{
      if (someLock.isLocked())
        someLock.unlock();
    }

java java.util.concurrent findbugs
person Geek    schedule 21.03.2013    source источник
comment
Зачем вы тестируете? Вы должны сделать someLock.lock(); перед попыткой.   -  person Denys Séguret    schedule 21.03.2013
comment
@dystroy Даже если есть тест, почему должно быть предупреждение?   -  person Geek    schedule 21.03.2013
comment
Я не знаю, я не вижу ошибочного пути, но эта конструкция кажется бессмысленной.   -  person Denys Séguret    schedule 21.03.2013

Ответы (3)


arrow_upward
5
arrow_downward

Если isLocked() что-то кинет, то не разблокируете.

Я не думаю, что isLocked может вызвать исключение, но когда вы блокируете, вы должны разблокировать, нет смысла тестировать. Так почему бы не использовать стандартный шаблон, описанный в javadoc :

someLock.lock();
try{
    //do something
} finally{
    someLock.unlock();
}

Так я сказал

  • это ложное срабатывание в том смысле, что ваш код не может не разблокировать
  • это настоящий позитив в том смысле, что вы должны исправить свой код в соответствии с рекомендацией
person Denys Séguret    schedule 21.03.2013

arrow_upward
3
arrow_downward

заблокировано

Запрашивает, удерживается ли эта блокировка каким-либо потоком. Этот метод предназначен для мониторинга состояния системы, не для управления синхронизацией.

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

someLock.lock();
try{      
 //do something
} finally{
  someLock.unlock();
}

or 

boolean locked=false;
try{      
 someLock.lock();
 locked=true;
 //do something
} finally{
  if (locked) someLock.unlock();
}
person Javier    schedule 21.03.2013
comment
Ну, это не совсем объясняет предупреждение. - person Denys Séguret; 21.03.2013
comment
@dystroy Я добавил некоторые пояснения. Я думаю, что Findbugs показывает предупреждение, потому что isLocked не предназначен для управления синхронизацией. - person Javier; 21.03.2013
comment
Второй код кажется бесполезным: если вы заблокируете, вы разблокируете, не нужно проверять. - person Denys Séguret; 21.03.2013
comment
@dystroy Если lock() находится в блоке try...finally; lock() может вызвать (непроверенное) исключение, и finally будет выполнено. - person Javier; 21.03.2013
comment
@Javier - вот почему лучше поместить вызов lock вне блока try. - person David Harkness; 22.03.2013

arrow_upward
1
arrow_downward

Это выглядит как ложное срабатывание, но я думаю, что могу понять, почему вы его получаете.

Правило FindBugs, скорее всего, закодировано для проверки того, что все пути вызывают unlock... независимо от того, действительно ли нужен вызов (с точки зрения состояния блокировки). Скорее всего, он не пытается отслеживать состояние блокировки и, скорее всего, не знает, что isLocked означает. Хотя для нас с вами очевидно, что нет необходимости вызывать unlock, если isLocked возвращает false, правило FindBugs не позволяет сделать такой вывод.

(Действительно, создание надежного вывода в широком диапазоне вариантов использования было бы сложной проблемой для разработчиков FindBugs. Мы находимся на территории «автоматического доказательства теорем» ...)

person Stephen C    schedule 21.03.2013