Assurer la métamorphose : analyse du projet Kafka

Vous êtes-vous déjà demandé quels bugs pourraient se cacher dans le code source des projets des entreprises mondiales ? Ne manquez pas l'occasion de repérer des bugs intéressants détectés par l'analyseur statique PVS-Studio dans le projet open source Apache Kafka.

Introduction

Apache Kafka est un projet open source bien connu principalement écrit en Java. LinkedIn l'a développé en 2011 en tant que courtier de messages, c'est-à-dire un pipeline de données pour différents composants du système. Aujourd'hui, c'est l'une des solutions les plus populaires de sa catégorie.

Prêt à jeter un œil sous le capot ?

P.S.

Je voulais juste laisser un petit mot sur le titre. Il fait référence à "La Métamorphose" de Franz Kafka, où le personnage principal se transforme en une monstrueuse vermine. Notre analyseur statique se bat pour empêcher vos projets de se transformer en vermine monstrueuse se transformer en un bug colossal, alors dites non à "La Métamorphose".

Oh non, des bugs

Tout humour est enraciné dans la douleur

Ce ne sont pas mes mots ; la citation appartient à Richard Pryor. Mais qu’importe ? La première chose dont je voudrais vous parler est une erreur stupide. Pourtant, après de nombreuses tentatives pour comprendre pourquoi le programme ne fonctionne pas correctement, il est frustrant de tomber sur quelque chose comme l'exemple ci-dessous :

@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);
  } 
  ....
}

Comme vous pouvez le constater, voici quelque chose qu'aucun développeur ne peut éviter : une faute de frappe triviale. Dans la toute première condition, les développeurs ont souhaité utiliser l'expression logique suivante :

keyFrom == null && keyTo == null

L'analyseur a émis deux avertissements :

V6001 Il y a des sous-expressions identiques 'keyFrom == null' à gauche et à droite de l'opérateur '&&'. ReadOnlyWindowStoreStub.java 327, ReadOnlyWindowStoreStub.java 327

V6007 L'expression 'keyFrom == null' est toujours fausse. ReadOnlyWindowStoreStub.java 329

Nous pouvons comprendre pourquoi. De telles fautes de frappe riantes sont intemporelles pour chaque développeur. Certes, nous pouvons passer beaucoup de temps à les chercher, mais ce ne sera pas un jeu d'enfant de se rappeler où ils se sont cachés.

Dans la même classe, il y a exactement la même erreur dans une autre méthode. Je pense qu'il est juste d'appeler cela du copypasta.

@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);
    }
  }
  ....
}

Voici les mêmes avertissements :

V6007 L'expression 'keyFrom == null' est toujours fausse. ReadOnlyWindowStoreStub.java 273

V6001 Il y a des sous-expressions identiques 'keyFrom == null' à gauche et à droite de l'opérateur '&&'. ReadOnlyWindowStoreStub.java 271, ReadOnlyWindowStoreStub.java 271

Ne vous inquiétez pas, nous n'aurons pas à examiner des centaines de lignes de code à la fois. PVS-Studio est excellent pour gérer des choses aussi simples. Que diriez-vous d’aborder quelque chose d’un peu plus difficile ?

Mutable synchronisé

À quoi sert le mot-clé synchronized en Java ? Ici, je me concentrerai uniquement sur les méthodes synchronisées, pas sur les blocs. Selon la documentation Oracle, le mot-clé synchronized déclare une méthode comme synchronisée pour garantir une interaction thread-safe avec une instance. Si un thread invoque une méthode synchronisée de l'instance, les autres threads qui tentent d'invoquer des méthodes synchronisées de la même instance seront bloqués (c'est-à-dire que leur exécution sera suspendue). Ils seront bloqués jusqu'à ce que la méthode invoquée par le premier thread traite son exécution. Ceci est nécessaire lorsque l'instance est visible par plusieurs threads. Les opérations de lecture/écriture de telles instances doivent être exécutées uniquement via des méthodes synchronisées.

Les développeurs ont enfreint la règle dans la classe Sensor, comme illustré dans le fragment de code simplifié ci-dessous. Les opérations de lecture/écriture sur le champ d'instance sont exécutées via des méthodes synchronisées et non synchronisées. Cela peut conduire à une condition de concurrence critique et rendre le résultat imprévisible.

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;
  }
}

L'avertissement de l'analyseur ressemble à ceci :

V6102 Synchronisation incohérente du champ 'metrics'. Pensez à synchroniser le terrain sur tous les usages. Capteur.java 49, Capteur.java 254

Si différents threads peuvent modifier l'état de l'instance en même temps, les méthodes qui le permettent doivent être synchronisées. Si le programme ne prévoit pas que plusieurs threads puissent interagir avec l'instance, il est inutile de synchroniser ses méthodes. Dans le pire des cas, cela peut même nuire aux performances du programme.

Il y a beaucoup de telles erreurs dans le programme. Voici un fragment de code similaire pour lequel l'analyseur a émis l'avertissement :

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
  );
}

L'avertissement de l'analyseur :

V6102 Synchronisation incohérente du champ 'prefixKeyFormatter'. Pensez à synchroniser le terrain sur tous les usages. LogicalKeyValueSegment.java 60, LogicalKeyValueSegment.java 247

Iterator, iterator, and iterator again...

In the example, there are two rather unpleasant errors within one line at once. I'll explain their nature within the part of the article. Here's a code snippet:

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);
    }
    ....
  }
}

That's what the analyzer shows us:

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

Now that's a big dilemma! What's going on here, and how should we address it?!

First, let's talk about collections and generics. Using the generic types of collections helps us avoid ClassCastExceptions and cumbersome constructs where we convert types.

If we specify a certain data type when initializing a collection and add an incompatible type, the compiler won't compile the code.

Here's an example:

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
  }
}

However, if we delete an incompatible type from our Set, no exception will be thrown. The method returns false.

Here's an example:

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
  }
}

It's a waste of time. Most likely, if we encounter something like this in the code, this is an error. I suggest you go back to the code at the beginning of this subchapter and try to spot a similar case.

Second, let's talk about the Iterator. We can talk about iterating through collections for a long time. I don't want to bore you or digress from the main topic, so I'll just cover the key points to ensure we understand why we get the warning.

So, how do we iterate through the collection here? Here is what the for loop in the code fragment looks like:

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

The for loop entry is just syntactic sugar. The construction is equivalent to this one:

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

We're basically working with the collection iterator. All right, that's sorted! Now, let's discuss ConcurrentModificationException.

ConcurrentModificationException is an exception that covers a range of situations both in single-threaded and multi-threaded programs. Here, we're focusing on single-threading. We can find an explanation quite easily. Let's take a peek at the Oracle docs: a method can throw the exception when it detects parallel modification of an object that doesn't support it. In our case, while the iterator is running, we delete objects from the collection. This may cause the iterator to throw a ConcurrentModificationException.

How does the iterator know when to throw the exception? If we look at the ArrayList collection, we see that its parent, AbstactList, has the modCount field that stores the number of modifications to the collection:

protected transient int modCount = 0;

Here are some usages of the modCount counter in the ArrayList class:

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;
}

So, the counter is incremented each time when the collection is modified.

Btw, the fastRemove method is used in the remove method, which we use inside the loop.

Here's the small code fragment of the ArrayList iterator inner workings:

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();
    }
  }
}

Let me explain that last fragment. If the collection modifications don't match the expected number of modifications (which is the sum of the initial modifications before the iterator was created and the number of the iterator operations), a ConcurrentModificationException is thrown. That's only possible when we modify the collection using its methods while iterating over it (i.e. in parallel with the iterator). That's what the second warning is about.

So, I've explained you the analyzer messages. Now let's put it all together:

We attempt to delete an element from the collection when the Iterator is still running:

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

However, since the incompatible element is passed to ArrayList for deletion (the remove method returns a UUID object from topicIds), the modification count won't increase, but the object won't be deleted. Simply put, that code section is rudimentary.

I'd venture to guess that the developer's intent is clear. If that's the case, one way to fix these two warnings could be as follows:

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);

Void, sweet void

Where would we go without our all-time favorite null and its potential problems, right? Let me show you the code fragment for which the analyzer issued the following warning:

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());
  }
}

If members doesn't contain an object with the memberId key, oldMember will be null. It can lead to a NullPointerException in the removeStaticMember method.

Boom! The parameter is checked for null:

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

The next error will be the last one in the article—I'd like to wrap things up on a positive note. The code below—as well as the one at the beginning of this article—has a common and silly typo. However, it can certainly lead to unpleasant consequences.

Let's take a look at this code fragment:

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

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

Yeah, that's right. The method actually accesses the input object first, and then checks whether it's referencing null.

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

Again, I'll note that such typos are ok. However, they can lead to some pretty nasty results. It's tough and inefficient to search for these things in the code manually.

Conclusion

In sum, I'd like to circle back to the previous point. Manually searching through the code for all these errors is a very time-consuming and tedious task. It's not unusual for issues like the ones I've shown to lurk in code for a long time. The last bug dates back to 2018. That's why it's a good idea to use static analysis tools. If you'd like to know more about PVS-Studio, the tool we have used to detect all those errors, you can find out more here.

That's all. Let's wrap things up here. "Oh, and in case I don't see ya, good afternoon, good evening, and good night."

I almost forgot! Catch a link to learn more about a free license for open-source projects.

Ce qui précède est le contenu détaillé de. pour plus d'informations, suivez d'autres articles connexes sur le site Web de PHP en chinois!