Скажем нет "Превращению", или анализ Kafka

• Вступление
• Итак, ошибки
  • Великая шутка и боль всегда идут бок о бок
  • Непостоянный synchronized
  • Итератор, итератор и ещё раз итератор...
  • Моя милая пустота
• Заключение

Бывало ли вам интересно, какие баги может таить исходный код проектов, которыми пользуются большие компании по всему миру? Не упустите шанс познакомиться с интересными ошибками, которые нашёл статический анализатор PVS-Studio в исходном коде Open Source проекта Apache Kafka.

Вступление

Apache Kafka — популярный Open Source проект, написанный преимущественно на Java. Представляет собой "брокер" сообщений, то есть шину данных для различных компонентов системы. Разработан компанией LinkedIn в 2011 году. На данный момент является одним из популярнейших решений в рамках своего жанра.

Ну что, готовы заглянуть под капот?

Итак, ошибки

Великая шутка и боль всегда идут бок о бок

Данная цитата, увы, не моя, она принадлежит Джону Леннону. Но причём она здесь? Первое, что я хотел бы показать вам по результатам анализа, было по-настоящему забавной, шуточной ошибкой. Однако после долгих попыток понять, почему программа не работает должным образом, наткнуться на то, что будет приведено в примере снизу — довольно-таки больно.

@Override
public KeyValueIterator<Windowed<K>, V> backwardFetch(
  K keyFrom,
  K keyTo,
  Instant timeFrom,
  Instant timeTo) {
  ....
  if (keyFrom == null && keyFrom == null) {   // <=
    kvSubMap = kvMap;
  } else if (keyFrom == null) {
    kvSubMap = kvMap.headMap(keyTo, true);
  } else if (keyTo == null) {
    kvSubMap = kvMap.tailMap(keyFrom, true);
  } else {
    // keyFrom != null and KeyTo != null 
    kvSubMap = kvMap.subMap(keyFrom, true, keyTo, true);
  } 
  ....
}

В данном фрагменте вы можете наблюдать то, что происходит по ходу разработки с каждым из нас — банальная опечатка. Очевидно, что в самом первом условии подразумевалось следующее логическое выражение:

keyFrom == null && keyTo == null

На этот участок кода анализатор выдал сразу два предупреждения:

V6001 There are identical sub-expressions 'keyFrom == null' to the left and to the right of the '&&' operator. ReadOnlyWindowStoreStub.java 327, ReadOnlyWindowStoreStub.java 327

V6007 Expression 'keyFrom == null' is always false. ReadOnlyWindowStoreStub.java 329

И оба справедливы. Такие забавные опечатки свойственны каждому разработчику, однако времени на их поиск можно потратить уйму. А вспоминаться оно будет, к сожалению, не лучшим образом.

В рамках этого же класса в другом методе присутствует точно такая же ошибка. С уверенностью можно сказать, что это копипаста.

@Override
public KeyValueIterator<Windowed<K>, V> fetch(
  K keyFrom,
  K keyTo,
  Instant timeFrom,
  Instant timeTo) {
  ....
  NavigableMap<K, V> kvMap = data.get(now);
  if (kvMap != null) {
    NavigableMap<K, V> kvSubMap;
    if (keyFrom == null && keyFrom == null) {      // <=
      kvSubMap = kvMap;
    } else if (keyFrom == null) {
      kvSubMap = kvMap.headMap(keyTo, true);
    } else if (keyTo == null) {
      kvSubMap = kvMap.tailMap(keyFrom, true);
    } else {
      // keyFrom != null and KeyTo != null
      kvSubMap = kvMap.subMap(keyFrom, true, keyTo, true);
    }
  }
  ....
}

И те же самые срабатывания:

V6007 Expression 'keyFrom == null' is always false. ReadOnlyWindowStoreStub.java 273

V6001 There are identical sub-expressions 'keyFrom == null' to the left and to the right of the '&&' operator. ReadOnlyWindowStoreStub.java 271, ReadOnlyWindowStoreStub.java 271

Но не стоит бояться, бегать глазами по сотням строк кода нам не придётся ни разу. С такими простыми вещами PVS-Studio справляется на ура. Как насчёт чего-нибудь посложнее?

Непостоянный synchronized

Для чего в Java используется ключевое слово synchronized? В рамках данной ошибки я буду говорить об этом лишь в контексте синхронизированных методов, не блоков. Согласно документации Oracle, ключевое слово synchronized позволяет обеспечивать потокобезопасное взаимодействие с объектом, объявляя метод синхронизированным. Если поток обращается к синхронизированному методу объекта, другие потоки, которые попытаются обратиться к синхронизированным методам этого же объекта, будут заблокированы (то есть, их выполнение приостановится). Заблокированными они будут до тех пор, пока метод, который был вызван первым потоком, не закончит своё выполнение. Необходимо это в тех случаях, когда объект виден более чем одному потоку. Операции записи и чтения у такого объекта должны осуществляться только через синхронизированные методы.

В классе Sensor, упрощённый фрагмент которого приведён ниже, данное правило было нарушено. Операции записи и чтения с полем объекта совершают как через синхронизированные методы, так и через несинхронизированные. Такое взаимодействие с объектом может повлечь за собой состояние гонки, и результат окажется непредсказуемым.

private final Map<MetricName, KafkaMetric> metrics;

public void checkQuotas(long timeMs) {                  // <=
  for (KafkaMetric metric : this.metrics.values()) {
    MetricConfig config = metric.config();
    if (config != null) {
      ....
    }
  }
  ....
}  

public synchronized boolean add(CompoundStat stat,      // <=
                                MetricConfig config) {       
  ....
  if (!metrics.containsKey(metric.metricName())) {         
    metrics.put(metric.metricName(), metric);            
  }  
  ....
}  

public synchronized boolean add(MetricName metricName,  // <=
                                MeasurableStat stat, 
                                MetricConfig config) {  
  if (hasExpired()) {
    return false;
  } else if (metrics.containsKey(metricName)) {
    return true;
  } else {
    ....
    metrics.put(metric.metricName(), metric);
    return true;
  }
}

Вот так выглядит срабатывание анализатора на данный участок кода:

V6102 Inconsistent synchronization of the 'metrics' field. Consider synchronizing the field on all usages. Sensor.java 49, Sensor.java 254

Если состояние объекта могут изменять сразу несколько потоков, то методы, посредствам которых это будет происходить, должны быть синхронизированными. В случае, если в программе не подразумевается, что с объектом могут взаимодействовать несколько потоков, делать его методы синхронизированными как минимум бессмысленно, а как максимум это может неблагоприятным образом повлиять на производительность вашей программы.

Такая ошибка в программе не единственная. Ниже приведён пример, где анализатор среагировал на точно такой же случай:

private final PrefixKeyFormatter prefixKeyFormatter; 

@Override
public synchronized void destroy() {                // <=
  ....
  Bytes keyPrefix = prefixKeyFormatter.getPrefix();
  ....
}

@Override
public void addToBatch(....) {                      // <=
  physicalStore.addToBatch(
    new KeyValue<>(
    prefixKeyFormatter.addPrefix(record.key),
    record.value
    ), batch
  );
} 

@Override
public synchronized void deleteRange(....) {        // <=
  physicalStore.deleteRange(
    prefixKeyFormatter.addPrefix(keyFrom),
    prefixKeyFormatter.addPrefix(keyTo)
  );
}

@Override
public synchronized void put(....) {                // <=
  physicalStore.put(
    prefixKeyFormatter.addPrefix(key),
    value
  );
}

Сообщение анализатора:

V6102 Inconsistent synchronization of the 'prefixKeyFormatter' field. Consider synchronizing the field on all usages. LogicalKeyValueSegment.java 60, LogicalKeyValueSegment.java 247

Итератор, итератор и ещё раз итератор...

А вот в данном примере в рамках одной строки будут сразу две довольно неприятные ошибки. О природе каждой из них я расскажу вам в рамках этой части статьи. Но сначала, конечно же, сам код:

private final Map<String, Uuid> topicIds = new HashMap(); 

private Map<String, KafkaFutureVoid> handleDeleteTopicsUsingNames(....) { 
  ....
  Collection<String> topicNames = new ArrayList<>(topicNameCollection);

  for (final String topicName : topicNames) {
    KafkaFutureImpl<Void> future = new KafkaFutureImpl<>();

    if (allTopics.remove(topicName) == null) {
      ....
    } else {
      topicNames.remove(topicIds.remove(topicName));      // <=
      future.complete(null);
    }
    ....
  }
}

Вот что нам на это говорит анализатор:

V6066 The type of object passed as argument is incompatible with the type of collection: String, Uuid. MockAdminClient.java 569

V6053 The 'topicNames' collection of 'ArrayList' type is modified while iteration is in progress. ConcurrentModificationException may occur. MockAdminClient.java 569

Вот это уже дилемма! Что же тут происходит, и что мы с этим будем делать?!

Во-первых, давайте поговорим о коллекциях и дженериках. Использование обобщённых типов в совокупности с коллекциями позволяет нам обезопасить себя и других от ClassCastException'ов, а также от громоздких конструкций, где мы занимаемся приведением типов.

В случае, если мы при инициализации коллекции указали, с каким типом данных она будет работать и попытались, к примеру, добавить в неё элемент несовместного типа, компилятор просто не скомпилирует такой код.

Пример:

public class Test {
  public static void main(String[] args) {
    Set<String> set = new HashSet<>();
    set.add("str");
    set.add(UUID.randomUUID()); // java.util.UUID cannot be converted to
                                // java.lang.String
  }
}

Однако, если попытаться удалить элемент несовместного типа из нашего Set'а, исключения не возникнет. Метод просто вернёт false.

Пример:

public class Test {
  public static void main(String[] args) {
    Set<String> set = new HashSet<>();
    set.add("abc");
    set.add("def");
    System.out.println(set.remove(new Integer(13))); // false
  }
}

Такое действие является бессмысленным. Скорее всего, если вы встречаетесь с чем-то подобным в коде — вы имеете дело с ошибкой. Советую вернуться к коду в начале этой подглавы и попробовать найти место, в котором ситуация эквивалентна вышеописанной.

Во-вторых, давайте поговорим про Iterator. Итерирование по коллекциям как тему можно развернуть очень и очень объёмно. Однако, чтобы вас не утомлять и самому не сбиться с курса, я постараюсь затронуть только самые важные моменты, которые необходимы для понимания сути данного срабатывания.

Итак, как мы здесь итерируемся по коллекции? Цикл for во фрагменте выглядит следующим образом:

for (Type collectionElem : collection) {
  ....
}

По своей сути, такая запись цикла for является "синтаксическим сахаром". Данная конструкция эквивалентна следующей:

for (Iterator<Type> iter = collection.iterator(); iter.hasNext();) {
  Type collectionElem = iter.next();
  ....
}

То есть, используя данную запись, мы неявно работаем с итератором коллекции. Отлично, с этим разобрались! Теперь немного поговорим о ConcurrentModificationException.

ConcurrentModificationException — исключение, которое охватывает определённый спектр ситуаций как в рамках однопоточных программ, так и многопоточных. Здесь нас интересует только часть про однопоточность. И за объяснением далеко идти не придётся. Предлагаю обратиться к документации Oracle: данное исключение может быть выброшено методом в том случае, когда была обнаружена параллельная модификация объекта, когда при этом сам объект таких модификаций не поддерживает. В нашем случае, мы параллельно работе итератора удаляем объекты из коллекции, на что итератор может выбросить ConcurrentModificationException.

А как итератор понимает, когда данное исключение выбрасывать? Если заглянуть в реализацию коллекции ArrayList, мы видим, что у её предка AbstactList есть поле modCount, которое хранит в себе количество модификаций коллекции:

protected transient int modCount = 0;

Примеры использования счётчика modCount в классе ArrayList:

public boolean add(E e) {
  modCount++;
  add(e, elementData, size);
  return true;
}

private void fastRemove(Object[] es, int i) {
  modCount++;
  final int newSize;
  if ((newSize = size - 1) > i)
    System.arraycopy(es, i + 1, es, i, newSize - i);
  es[size = newSize] = null;
}

То есть, при каждой модификации коллекции счётчик инкрементируется.

Метод fastRemove, кстати, как раз и используется в методе remove, который мы и используем внутри цикла.

А вот небольшой фрагмент внутренностей ArrayList-итератора:

private class Itr implements Iterator<E> {
  ....
  int expectedModCount = modCount;            

  final void checkForComodification() {
  if (modCount != expectedModCount)               // <=
    throw new ConcurrentModificationException();
  }

  public E next() {
    checkForComodification();              
    ....
  }
    
  public void remove() {
    ....
    checkForComodification();             

    try {
      ArrayList.this.remove(lastRet);   
      ....
      expectedModCount = modCount;
    } catch (IndexOutOfBoundsException ex) {
      throw new ConcurrentModificationException();
    }
  }
  ....
  public void add(E e) {
    checkForComodification();            
    try {
      ....
      ArrayList.this.add(i, e);        
      ....
      expectedModCount = modCount;     
    } catch (IndexOutOfBoundsException ex) {
      throw new ConcurrentModificationException();
    }
  }
}

Объяснение последнего фрагмента: в случае, если количество модификаций, которое было с коллекцией, не совпадает с ожидаемым количеством модификаций (а ожидаемое — это то, которое было до создания итератора + то, что с ней совершил итератор), то выбрасывается ConcurrentModificationException. А возможно такое только в случае, если мы во время итерирования коллекции итератором модифицировали её напрямую её же методами (то есть, параллельно итератору). Это всё, что касается второго срабатывания.

Так, про сами сообщения анализатора я вам рассказал. Теперь давайте соединим всё в одну кучу:

Мы параллельно работе Iterator'a пытаемся удалить элемент из коллекции:

topicNames.remove(topicIds.remove(topicName)); 
// topicsNames – Collection<String>
// topicsIds – Map<String, UUID>

Однако поскольку на удаление в ArrayList передаётся несовместный элемент (topicIds в результате выполнения метода remove вернёт UUID объект), счётчик модификаций не увеличится, но и объект не удалится. Проще говоря, на данный момент тот участок кода, что я привёл, является рудиментарным.

Рискну предположить, что замысел разработчика мне понятен. В таком случае, вариант исправления двух обозреваемых срабатываний мог бы выглядеть, к примеру, так:

Collection<String> topicNames = new ArrayList<>(topicNameCollection);

List<String> removableItems = new ArrayList<>();

for (final String topicName : topicNames) {
  KafkaFutureImpl<Void> future = new KafkaFutureImpl<>();

  if (allTopics.remove(topicName) == null) {
    ....
  } else {
    topicIds.remove(topicName);
    removableItems.add(topicName);
    future.complete(null);
  }
  ....
}
topicNames.removeAll(removableItems);

Моя милая пустота

Ну и конечно же, куда без всеми любимого null и потенциальных проблем, которые связаны c ним. Представляю вам на обозрение участок кода, на который анализатор среагировал следующим образом:

V6008 Potential null dereference of 'oldMember' in function 'removeStaticMember'. ConsumerGroup.java 311, ConsumerGroup.java 323

@Override
public void removeMember(String memberId) {
  ConsumerGroupMember oldMember = members.remove(memberId);
  ....
  removeStaticMember(oldMember);
  ....
}

private void removeStaticMember(ConsumerGroupMember oldMember) {
  if (oldMember.instanceId() != null) {
    staticMembers.remove(oldMember.instanceId());
  }
}

В случае, если в members не содержится объект с ключом memberId, oldMember будет ссылаться на null. Это чревато выбросом NullPointerException в методе removeStaticMember.

Вжух, и параметр проверен на null:

if (oldMember != null && oldMember.instanceId() != null) {

А следующая ошибка в рамках данной статьи будет последней, а закончить хочется на приятной ноте. Код, который представлен ниже (как и код в начале статьи) представляет собой обычную, забавную, свойственную каждому из нас опечатку. Впрочем, привести к неприятным последствиям она уж точно способна.

Итак, предлагаю взглянуть:

protected SchemaAndValue roundTrip(...., SchemaAndValue input) {
  String serialized = Values.convertToString(input.schema(),
                                             input.value());

  if (input != null && input.value() != null) {   
    ....
  }
  ....
}

Да, вы не ошиблись. В данном методе действительно сначала к объекту input обращаются, а потом проверяют, не ссылается ли он на null.

V6060 The 'input' reference was utilized before it was verified against null. ValuesTest.java 1212, ValuesTest.java 1213

Опять же, упомяну, что такие опечатки — нормальны. Однако привести они могут к очень неприятным последствиям. И глазами искать в коде такие вещи очень и очень тяжело, а главное — весьма непродуктивно.

Заключение

В заключение хотелось бы ещё раз уделить внимание предыдущей мысли. Самому бегать по коду и искать все ошибки, что я привёл — занятие весьма затратное и не самое приятное. Вещи, подобные тем, что я показал, способны сидеть в коде очень и очень долго (к примеру, последняя ошибка была закоммичена аж в 2018 году). Именно поэтому использовать возможности статического анализа — можно и нужно. За более подробной информацией о PVS-Studio, с помощью которого поиск всех вышеупомянутых ошибок был реализован, рекомендую обратиться сюда.

На этом у меня всё, буду с вами прощаться. "И на случай, если я вас больше не увижу — добрый день, добрый вечер и доброй ночи".

Ах, да, чуть не забыл! Ловите ссылку на информацию о бесплатном лицензировании для Open Source проектов.