3 200 % d’utilisation CPU

(josephmate.github.io)

2 points par GN⁺ 2025-03-01 | 1 commentaires | Partager sur WhatsApp

Dans une application Java 17, les 32 cœurs ont tous été saturés, faisant grimper l’utilisation CPU jusqu’à 3 200 %; le tri du thread dump par CPU time a montré que plusieurs threads étaient bloqués dans TreeMap.put()
Le code de boucle soupçonné au départ était inefficace, mais sa complexité O(N lg(M)) et la taille des entrées ne suffisaient pas à expliquer un temps d’exécution digne d’un incident
La cause réelle était que plusieurs threads modifiaient une TreeMap partagée sans protection, créant un cycle dans la structure interne du red-black tree, ce qui faisait partir recherches et insertions en boucle infinie
Le même phénomène est apparu dans un code de reproduction simple, avec ExecutorService et dans un service gRPC ; le pool de threads ne faisait pas apparaître la NPE sur la sortie standard, ce qui a rendu l’identification de la cause encore plus difficile
La correction ne se résume pas à passer à Collections.synchronizedMap ou ConcurrentHashMap : il faut aussi prévoir la gestion des exceptions de l’executor, des alertes CPU et NPE, de l’analyse statique et des tests multithread

Symptômes de l’incident et premier indice

La machine était presque hors d’usage, au point que même une connexion ssh était difficile, et l’utilisation CPU est montée jusqu’à 3 200 %
- Les 32 cœurs de l’hôte étaient tous utilisés
- C’était une situation différente d’un précédent bug qui n’utilisait qu’un seul cœur à 100 %
Le thread dump du runtime Java 17 incluait le CPU time, et un tri selon ce CPU time a fait ressortir des threads similaires
- La pile pointait vers java.util.TreeMap.put()
- L’emplacement dans le code applicatif était BusinessLogic.someFunction(BusinessLogic.java:29)

Le code soupçonné au départ et les hypothèses écartées

Au point problématique, le code parcourait unrelatedObjects alors que le corps de la boucle n’utilisait que relatedObject

for (SomeOtherType unrelatedObject : unrelatedObjects) {
    treeMap.put(relatedObject.a(), relatedObject.b()); // line 29
}

Ce code pouvait être réduit à un seul put, comme ci-dessous

treeMap.put(relatedObject.a(), relatedObject.b());

Comme unrelatedObjects était utilisé plus loin dans la fonction, le paramètre lui-même ne pouvait pas être supprimé
Il était possible que l’utilisation de unrelatedObject ait disparu lors d’un refactoring
Même en faisant grossir treeMap et unrelatedObjects à 1 000 000 d’entrées chacun dans les tests unitaires, le problème n’a pas pu être reproduit
- Si la taille de unrelatedObjects est N et celle de treeMap est M, la complexité est O(N lg(M))
- Il a été estimé qu’il faudrait plutôt de 100 millions à 1 milliard d’entrées pour observer un temps d’exécution de l’ordre d’une minute
- Cela ne collait pas non plus avec l’hypothèse selon laquelle, dans l’application réelle, les deux structures de données ne dépassaient pas 1 000 éléments

La boucle infinie créée par une TreeMap non protégée

La définition du champ TreeMap était la suivante

private final Map<K,V> treeMap = new TreeMap<>();

Plusieurs threads accédaient à cette TreeMap, mais il n’y avait ni synchronisation ni mécanisme de protection
La TreeMap de Java est implémentée comme un red-black tree, et si des modifications concurrentes cassent les liens internes entre nœuds, un cycle peut apparaître
Lors d’une recherche ou de l’insertion d’une valeur encore absente, le code peut suivre ce cycle et tomber dans une boucle infinie

Expérience de reproduction simple

Une expérience a été écrite dans laquelle plusieurs threads mettaient à jour aléatoirement une TreeMap partagée

for (int i = 0; i < numThreads; i++) {
    threads.add(new Thread(() -> {
        Random random = new Random();
        for(int j = 0; j < numUpdates; j++) {
            try {
                treeMap.put(random.nextInt(1000), random.nextInt(1000));
            } catch (NullPointerException e) {
                // let it keep going so we can reproduce the issue.
            }
        }
    }));
}

Le projet complet se trouve dans SimpleRepro.java
Au début, le try/catch semblait être l’élément clé
- Sans try/catch, certains threads mouraient sur une NullPointerException et le programme s’arrêtait
- Après avoir ajouté le try/catch, quelques exécutions ont permis d’observer une utilisation CPU de 500 %
Une race condition ne conduit pas seulement à une corruption de données ou à un deadlock : elle peut aussi endommager une structure de données de façon à permettre une boucle infinie, et donc entraîner un incident de performance

Vérification du cycle interne dans TreeMap

Une expérience a été écrite avec reflection pour accéder aux champs root, left, right, key et color de TreeMap, afin d’afficher les nœuds et leurs couleurs
Si, pendant le parcours, une TreeMap.Entry déjà visitée est rencontrée à nouveau, cela est considéré comme un cycle

private void print(
    Object treeMapEntry, String tabs, IdentityHashMap<Object, Object> visited
) throws Exception {
    if (treeMapEntry != null && !visited.containsKey(treeMapEntry)) {
        visited.put(treeMapEntry, treeMapEntry);
        print(treeMapEntryLeft.get(treeMapEntry), tabs + " ", visited);
        System.out.println(tabs + treeMapEntryKey.get(treeMapEntry) + ":"
            + (treeMapEntryColor.getBoolean(treeMapEntry) ? "BLACK" : "RED"));
        print(treeMapEntryRight.get(treeMapEntry), tabs + " ", visited);
    } else if (treeMapEntry != null && visited.containsKey(treeMapEntry)) {
        System.out.println(tabs + treeMapEntryKey.get(treeMapEntry) + ":"
            + (treeMapEntryColor.getBoolean(treeMapEntry) ? "BLACK" : "RED")
            + " CYCLE"
        );
    }
}

Pour ouvrir les restrictions d’accès des modules Java, l’argument JVM suivant était nécessaire à l’exécution

--add-opens java.base/java.util=ALL-UNNAMED

Cas similaires existants et différences

Le même type de problème était déjà connu
Ce cas examine aussi du code de reproduction réel, des tentatives de reproduction dans plusieurs langages, ainsi que des idées de modification de TreeMap et du red-black tree

Reproduction plus réaliste avec ExecutorService

Un code qui ignore simplement les NPE peut sembler peu réaliste, mais dans ExecutorService, les exceptions peuvent facilement ne pas apparaître

final ExecutorService pool = Executors.newFixedThreadPool(numThreads);
final TreeMap<Integer,Integer> treeMap = new TreeMap<>();
Random random = new Random();

for (int i = 0; i < numThreads*numUpdatesPerThread; i++) {
    pool.submit(() -> {
        treeMap.put(random.nextInt(10000), random.nextInt(10000));
    });
}

Le code complet se trouve dans ExecutorUncaughtRepro.java
À l’exécution, le programme se fige, et le thread dump montre un thread bloqué dans TreeMap.put()
Il n’y avait rien sur la sortie standard
- Le pool de threads avalait la NPE, ne laissant apparaître aucun signal du problème
- La situation réelle était identique
Si vous gérez directement un thread pool, les mesures suivantes sont nécessaires
- Enregistrer un uncaught exception handler via une thread factory
- Traiter le Future retourné et vérifier la NPE encapsulée dans une ExecutionException avec future.get()

Même problème dans un service gRPC

Dans un service basé sur un thread pool, comme un service gRPC, une TreeMap non protégée peut créer le même problème

@Override
public void addReceipt(
    ReceiptProcessorServiceOuterClass.AddReceiptRequest req,
    StreamObserver<ReceiptProcessorServiceOuterClass.AddReceiptResponse> responseObserver
) {
    int timestamp = req.getTimestamp();
    int totalPrice = req.getTotalPrice();
    receipts.put(timestamp, totalPrice);
    ReceiptProcessorServiceOuterClass.AddReceiptResponse response =
        ReceiptProcessorServiceOuterClass.AddReceiptResponse.newBuilder().build();
    responseObserver.onNext(response);
    responseObserver.onCompleted();
}

Le code complet se trouve dans GrpcRepro.java
La définition protobuf se trouve dans ReceiptProcessorService.proto
Le thread dump montre des threads grpc-default-executor-* bloqués en état runnable dans TreeMap.put()

Hypothèses sur la cause et expériences par langage

Parmi les causes possibles, l’hypothèse retenue est que deux threads effectuent indépendamment des rotations de l’arbre dans des directions opposées, ou que les écritures de rotations qui se chevauchent s’intercalent et créent un cycle
Il n’y a toutefois pas de preuve indiquant quel interleaving de threads crée le cycle
Au début, la NPE semblait nécessaire, mais les expériences suivantes ont invalidé cette hypothèse
Des tentatives de reproduction du même problème ont été faites dans plusieurs langages
- Java étant la base de tout le cas, la reproduction y fonctionne
- Le std::map de C++ utilise un red-black tree et, rarement, au lieu d’un segfault, un thread se fige avec une forte utilisation CPU
- Go s’est également reproduit contrairement aux attentes, et le code se trouve dans l’expérience Go
- Ruby n’a pas pu être reproduit malgré la possibilité d’y attraper une NPE, et il est possible que le GIL empêche l’interleaving qui crée le problème
Dans l’expérience C++, contrairement aux attentes, la boucle infinie s’est reproduite même sans try/catch ni exception de pointeur nul
- Parfois, le programme se terminait par un segmentation fault
- Très rarement, l’exécution restait bloquée plus de 10 minutes, et top montrait que SimpleRepro utilisait 170,8 % de CPU
- En C++, une référence à un pointeur nul provoque un segfault ; il doit donc exister un interleaving ne passant pas par null
Après avoir observé ce résultat, l’expérience Java a été relancée 12 fois sans attraper les NPE, et une boucle infinie dans TreeMap.put() a aussi été reproduite sans catch de NPE

Correction simple et défense au niveau de la structure de données

La correction la plus simple consiste à protéger la TreeMap partagée
- L’envelopper avec Collections.synchronizedMap
- La remplacer par ConcurrentHashMap et ne trier qu’au moment où c’est nécessaire
Une défense plus controversée consiste à suivre les nœuds visités pendant le parcours du red-black tree
- Si un nœud déjà visité est rencontré de nouveau, lever une ConcurrentModificationException
- Cela n’empêche pas la corruption des données elle-même
- Cela peut empêcher l’utilisation CPU à 100 % causée par une boucle infinie
- La mémoire supplémentaire est limitée à la hauteur de l’arbre, soit O(lg(n))
- La hauteur d’un red-black tree est garantie en O(lg(n))
- Il est peu probable que la bibliothèque standard adopte cette approche
L’exemple de correction enregistre les nœuds visités avec une IdentityHashMap dans getEntry et put

IdentityHashMap<Entry<?,?>, Boolean> visited = new IdentityHashMap<>();

while (p != null) {
    visited.put(p, true);
    int cmp = k.compareTo(p.key);
    if (cmp < 0)
        p = p.left;
    else if (cmp > 0)
        p = p.right;
    else
        return p;

    if (visited.containsKey(p)) {
        throw new ConcurrentModificationException("TreeMap corrupted. Loop detected");
    }
}

Le code complet de la correction se trouve dans ProtectedTreeMap.java

Empiler plusieurs couches de défense

Les erreurs arrivent ; une seule couche de défense ne suffit donc pas
Dans ce cas, plusieurs erreurs se sont combinées, mais une partie de la surveillance restait en place, ce qui a permis de découvrir le problème
Alertes NPE
- Il n’y avait pas d’alerte sur l’apparition de NPE elle-même
- Il n’y avait que des alertes sur le taux d’erreur, et cette NPE ne se produisait qu’une fois par thread worker de handler API, sans dépasser le seuil de taux d’erreur
- À cause du mode de traitement de l’executor, aucun log de NPE n’a été conservé
Alertes sur les anomalies d’utilisation CPU
- L’utilisation CPU était surveillée avec des alertes basées sur un simple seuil
- Quand l’utilisation CPU a dépassé ce seuil, le comportement a été considéré comme anormal, une alerte a été déclenchée, et c’est par ce chemin que le problème a été découvert
Gestion des exceptions dans l’executor
- Si vous soumettez des tâches à un executor, vous devez impérativement configurer un uncaught exception handler
```
thread.setUncaughtExceptionHandler(
    (dyingThread, throwable) -> {
        logger.error("uncaught exception!", throwable);
    }
);
```
- Au lieu d’une ThreadFactory écrite à la main, vous pouvez utiliser Apache Commons BasicThreadFactory ou Guava ThreadFactoryBuilder
- Sans handler, les exceptions sont avalées et la surveillance des logs ne peut pas non plus détecter le problème
Revue de code et analyse statique
- Une revue de code aurait pu repérer la combinaison threads + TreeMap, ou proposer de ne pas utiliser TreeMap si le tri n’était pas nécessaire, mais cela n’a pas été le cas ici
- Des outils d’analyse statique comme SpotBugs, JLint ou Chord peuvent détecter ce type de problème de concurrence au moment du build
- La ressource How Good is Static Analysis at Finding Concurrency Bugs? est citée à propos de l’analyse statique
Tests
- Il n’y avait pas de tests multithread sur ce chemin de code
- Si un code peut être soumis à des accès concurrents, les tests doivent aussi couvrir des situations multithread

Conclusion

Des modifications concurrentes non protégées peuvent se manifester non seulement par de la corruption de données, mais aussi par des boucles infinies et une forte utilisation CPU
Si plusieurs threads modifient simultanément une structure de données reposant sur des pointeurs internes, comme TreeMap, la structure du red-black tree peut créer un cycle
En combinant choix d’une structure de données synchronisée, gestion des exceptions, alertes, analyse statique et tests multithread, il est possible de détecter ou de prévenir ce problème plus rapidement

1 commentaires

GN⁺ 2025-03-01

Avis sur Hacker News

Il est bien connu que les collections de base de Java ne sont pas thread-safe par conception ; cela aurait donc dû être détecté
L’OP devrait aussi parcourir le reste du code pour vérifier s’il existe d’autres endroits où plusieurs threads pourraient manipuler des collections en même temps
En l’enveloppant avec Collections.synchronizedMap ou en le remplaçant par ConcurrentHashMap, les opérations individuelles sur la map deviennent thread-safe, mais cela ne garantit pas que des séquences d’opérations le soient aussi
On peut aussi douter que l’objet qui possède le TreeMap soit lui-même thread-safe, et une modification consistant à suivre les nœuds visités ne ferait que laisser la collection non sûre échouer de façon plus subtile
Le point essentiel n’est pas un effet de bord de TreeMap, mais la violation du contrat de la collection ; même en remplaçant par HashMap, le code resterait incorrect
Dans du code manipulé par plusieurs threads, l’approche la plus fiable a été de rendre immuables tous les objets possibles, et de limiter les objets qui ne peuvent pas l’être à de petites zones indépendantes et fortement contrôlées
- Ce point est vraiment important : même avec un tableau dynamique thread-safe, la combinaison de deux opérations — vérifier la longueur avec size(), puis accéder à element(10) — n’est pas atomique
  Si un autre thread supprime un élément entre les deux appels, on peut se retrouver avec un accès hors limites
  La solution consiste à utiliser une méthode atomique qui fait les deux à la fois, par exemple element_or_null(), ou bien à utiliser dès le départ un tableau classique et à protéger l’ensemble des deux opérations par un mutex
- Une course de données survient sur un état mutable partagé non synchronisé
  Comme les trois conditions sont nécessaires, il y a aussi trois solutions : ajouter de la synchronisation, par exemple avec des verrous ; ne pas partager l’accès mutable, comme dans un modèle à propriétaire unique utilisant des canaux ; rendre les données immuables, comme l’enseigne la programmation purement fonctionnelle
  Je crois que Google a aussi beaucoup investi dans ce modèle avec Guava
  Rust vous laisse choisir laquelle de ces trois conditions abandonner, et empêche statiquement qu’elles soient toutes réunies en même temps
- Ma première réaction a aussi été : « une seconde, cette structure de données est-elle thread-safe ? », et au final cela ressemble à un choix de structure de données inadapté à l’objectif
  Forcer l’adaptation avec un mutex crée généralement d’autres problèmes et des goulots d’étranglement
  Pour utiliser des objets immuables, il faut employer des structures de données immuables exploitant le partage structurel afin d’éviter, ou au moins de réduire, une explosion de l’utilisation mémoire
  Avec des structures de données purement fonctionnelles, une partie du problème disparaît, mais si les threads dépendent des versions intermédiaires produites les uns par les autres, cela crée un autre problème épineux et peut nécessiter une autre structure de données
  Si l’on veut à tout prix conserver la structure de données mutable déjà utilisée, on peut sérialiser les tentatives d’accès avant qu’elles n’atteignent la structure, puis regrouper les accès en transactions afin de n’exécuter que des transactions complètes ; à ce stade, on a presque l’impression d’implémenter une base de données
- Je pense que le modèle de threading lui-même, qui tente une exécution indépendante dans un environnement à mémoire partagée, est fondamentalement erroné
  À mon avis, les efforts nécessaires pour faire fonctionner cela auraient dû être investis dans un meilleur modèle de processus
L’exemple simplifié du code dans l’article original n’est pas exact
Le code d’origine n’exécute treeMap.put que lorsque unrelatedObjects n’est pas vide, et cette différence peut être un bug ou non
Il faut aussi vérifier si a et b renvoient toujours les mêmes valeurs, et si treeMap se comporte vraiment uniquement comme une map
Par exemple, s’il s’agit d’une map qui journalise les mises à jour, il faut se demander s’il est acceptable de la modifier pour ne journaliser qu’une seule fois
- C’est une remarque juste. Il faudrait remplacer cela par un if qui vérifie que ce n’est pas vide
Une boucle infinie peut aussi apparaître si l’implémentation de Comparator ou de Comparable ne fournit pas un ordre total cohérent : https://stackoverflow.com/questions/62994606/concurrentskips...
Cela n’a rien à voir avec la concurrence
Son apparition peut dépendre des données concrètes traitées et de l’ordre de traitement, si bien que le code peut sembler fonctionner longtemps avant d’exploser en production
- Je me demande si quelqu’un l’a déjà vu en vrai. Cela ferait un bon sujet de billet de blog
  Personnellement, je n’ai encore jamais rencontré de comparateur bogué dépourvu d’ordre total
Je pensais que les conditions de concurrence ne pouvaient provoquer que de la corruption de données ou des interblocages, pas des problèmes de performance
Mais si elles peuvent corrompre des données au point de créer une boucle infinie, ça se tient
C’est pourquoi je pense souvent qu’en principe, il faut corriger les erreurs, comportements anormaux et avertissements d’un projet. Parce qu’ils peuvent causer des problèmes qui, en apparence, n’ont rien à voir
Cela dit, il est rare que ce principe soit accepté par ceux qui décident de ce qu’il faut faire
- C’est une bonne règle empirique, mais il faut aussi du pragmatisme
  Dans certains projets, le coût de suppression de toutes les erreurs, comportements anormaux et avertissements peut être largement supérieur au coût d’accepter quelques problèmes occasionnels qui semblent sans rapport
  Comme il est presque impossible de prédire la probabilité qu’une erreur donnée contribue à un futur problème, ou si la corriger à l’avance coûtera moins cher, on finit par être plus proche de l’art que de la science
  « Ne rien corriger » est terrible, et « tout corriger » est généralement irréaliste ; il faut donc un cadre de décision, une intuition issue de l’expérience, et la confiance des parties prenantes
- Au travail, nous traitons tous les avertissements comme des erreurs, et il est impossible de fusionner une pull request tant que tous les pipelines CI exécutés automatiquement ne passent pas
  Il faut de la discipline, mais une fois qu’on a atteint cet état, il est beaucoup plus facile de le maintenir
- Il est aussi important de ne pas attraper les exceptions pour les ignorer
  À moins de savoir concrètement qu’il est sûr de continuer l’exécution du programme, « attraper et seulement journaliser » est aussi une mauvaise idée
  Mieux vaut laisser l’exception se propager jusqu’à un endroit où un traitement utile est possible, par exemple renvoyer un 500 ou afficher une boîte de dialogue d’erreur
- Au final, il faut choisir ses combats
  Sur un projet Rails que je maintiens, les logs étaient remplis de « unsupported parameters » ; j’ai vérifié soigneusement les contrôleurs et autorisé ce qu’il fallait, mais ça continuait d’apparaître
  C’était probablement inoffensif, mais ça ajoutait beaucoup de bruit dans les logs
  Plusieurs personnes ont essayé de résoudre le problème, mais il y avait toujours une priorité plus élevée, et il était difficile de justifier de passer beaucoup de temps sur quelque chose sans impact fonctionnel
  C’est un problème pénible, comme des hémorroïdes. Il faut choisir entre se faire opérer et souffrir beaucoup pendant quelques semaines, ou simplement vivre avec ; c’est en général bénin, mais ça peut aussi s’aggraver et devenir un gros problème
  On pourrait appeler ce genre de phénomène des hémorroïdes numériques
- Même sans corruption, une condition de concurrence peut faire exécuter plusieurs fois la même tâche et ne laisser qu’un seul résultat, provoquant de gros problèmes de performance
  Si l’on juge qu’un avertissement est sans rapport, il vaut au minimum mieux l’expliquer dans un commentaire et, si possible, désactiver l’avertissement avec un pragma limité au périmètre le plus étroit possible
  Je préfère supprimer les comportements anormaux. Il m’est arrivé que du code marqué « je ne sais pas pourquoi ça marche » finisse par ne plus fonctionner ; si on l’avait nettoyé plus tôt, on aurait eu le temps de le corriger prudemment, au lieu de devoir le réécrire dans l’urgence
La partie où il est dit qu’ils ont « réussi à peine à se connecter en ssh » me rappelle une situation en master/doctorat
Dans notre labo, un petit groupe d’environ huit personnes travaillant sur le traitement parallèle et distribué partageait une machine, probablement une Sun UltraSparc 170, avec un disque dur de 1 Go et 128 ou 256 Mo de RAM
Il faut aussi tenir compte du fait qu’on redémarrait très rarement les machines Sun
Un nouvel étudiant avait apparemment pris un gros fichier texte, l’avait découpé en 32 ou 64 plages selon les numéros de ligne, puis, sans créer de fichiers séparés, avait lancé en parallèle N copies de perl, chacune chargée de traiter sa propre plage de lignes
Pour l’époque, les N interpréteurs perl consommaient beaucoup de RAM, et dès que le swap s’enclenchait, ils se mettaient à faire des seeks frénétiques vers différentes parties du même fichier pendant que chaque processus essayait de lire quelques lignes de plus
En plus, il est très probable que le J-ième processus devait lire J/N du fichier pour atteindre sa propre plage
Sur la console, il était impossible d’obtenir même l’invite de connexion, mais heureusement une session était déjà ouverte, et su a fini par afficher l’invite de mot de passe après 20 à 30 minutes
Après 5 à 10 minutes de plus, nous avons obtenu une session root, identifié la cause avec top, tenté de contacter l’utilisateur, puis tué les processus problématiques, et le système est revenu à la normale
L’idée en elle-même était correcte, mais c’était un cas où les limites du système n’avaient absolument pas été comprises ; à cause du disque dur et de la faible quantité de RAM, le goulot d’étranglement I/O était extrême, et un simple traitement linéaire aurait été bien plus efficace
En Java comme dans d’autres langages, effectuer des opérations concurrentes sur un objet non thread-safe produit certains des bugs les plus fascinants qui soient
- C’est pourquoi, quand on utilise un objet non thread-safe depuis plusieurs threads, il faut gérer soi-même l’accès atomique ou utiliser une version thread-safe
  Les erreurs de multithreading sont un cauchemar à déboguer
  Dans ce cas, c’était très facile à identifier dès la conception, et le fait de vouloir utiliser un conteneur ordinaire dans un environnement multithread aurait dû allumer tous les voyants d’alerte
- J’ai rencontré pas mal de bugs de concurrence, mais je n’ai jamais volontairement reproduit les incohérences qui apparaissent quand on retire le modificateur volatile d’un champ mutable en Java
  Peut-être que la JVM utilisée à l’époque était simplement trop bien faite
- Même lorsque j’étais convaincu de pouvoir accepter les conséquences du partage de quelque chose de non synchronisé, le résultat réel était toujours inattendu
- C’est un avis un peu radical, mais pour cette raison je préfère déboguer les conditions de concurrence en C/C++
  Le langage définit certes une sémantique délirante et pratiquement dépourvue de sens en cas de concurrence, mais en pratique cela se manifeste souvent par de la corruption mémoire ou des problèmes bruyants
  Comme la plupart des conditions de concurrence sont illégales, on peut créer des outils comme des vérificateurs de threads sans devoir modifier le code source pour en annoter le sens
  À l’inverse, Java n’a pas de comportement indéfini, mais il est facile que deux champs se désalignent subtilement, et ce genre de problème est bien plus difficile à traquer
- Une partie, voire la majorité, des opérations des Java Collections effectuent des contrôles d’intégrité pour signaler ce genre de problème
  Par exemple, une map peut lancer une ConcurrentModificationException
J’ai déjà vu le même phénomène en production avec du C#
Les symptômes étaient les mêmes, et l’analyse du dump du processus a montré un dictionnaire corrompu
Je pensais qu’on utilisait partout ConcurrentDictionary, mais un dictionnaire venu d’une bibliothèque était en cause
À l’époque, nous utilisions .NET Framework, et si je me souviens bien, .NET Core contient du code de détection des modifications concurrentes
Je ne connais pas l’implémentation, mais un simple compteur de version devrait suffire
Il est étrange de s’acharner comme si une NPE était l’ingrédient essentiel. Elle ne semble pas faire partie du scénario d’origine, et le seul fait qu’il n’y ait pas d’exception ne permet pas de conclure que ce genre de bug n’existe pas en C
Le cœur du problème, ce sont les invariants de classe. En général, pendant l’exécution d’un mutateur, les invariants ne sont pas vérifiés, et ils ne sont rétablis qu’à la fin
Si un autre mutateur s’exécute avant que les invariants soient restaurés, la structure de données est corrompue. Si l’on ne part pas d’un état valide, il est peu probable qu’on termine dans un état valide
- Au final, c’était dû à un raisonnement erroné
  Comme j’ai eu la malchance de ne pas réussir à reproduire le problème avec une NPE non interceptée, j’en ai conclu à tort qu’une NPE non interceptée était une condition nécessaire
J’ai déjà vu la même chose avec un java.util.HashMap insuffisamment synchronisé
C’était vers 2009, mais à ma connaissance c’est encore possible aujourd’hui
Si je me souviens bien, HashMap utilise le chaînage pour résoudre les collisions, et il me semble qu’un cycle s’était formé dans une chaîne
Cela dit, plutôt que de creuser pour le vérifier, on s’est concentrés sur la suppression pure et simple du code problématique
On pose souvent des questions de concurrence en entretien, et si l’on considère les data races comme un problème mineur, il est difficile de faire bonne impression ; cet exemple montre pourquoi
- Je ne savais pas que ce genre de chose pouvait arriver aussi avec HashMap
  C’est lié aux listes chaînées utilisées pour gérer les collisions ?
Je me demande si l’on ne pourrait pas ajouter un compteur incrémental pour détecter les cycles, et lever une exception s’il dépasse la profondeur de l’arbre ou la taille de la collection
Contrairement à l’approche par ensemble de hachage proposée par l’auteur, cela n’entraîne quasiment aucun surcoût mémoire ou CPU, et semble plus susceptible d’être accepté
Cela dit, en plus de 10 ans de C#, je n’ai jamais oublié de prendre en compte les accès concurrents aux structures de données dans des situations concurrentes
- Ce n’est pas une mauvaise idée, mais ajouter une précondition utile en présence d’une data race est généralement assez difficile
  Il y a beaucoup d’autres façons pour un arbre d’être corrompu
- Bonne idée. Je l’ai déjà fait pour des recherches binaires et des structures d’arbres
  Dans la mesure du possible, il faut éviter les boucles infinies, et les cas où ce n’est pas possible sont assez rares
  Ce n’est pas une correction, mais c’est une bonne stratégie d’atténuation
  Les boucles infinies font partie des bugs les plus pénibles. Elles sont faciles à trouver dans un débogueur, mais leurs conséquences peuvent être très ingrates, comme dans la situation de l’OP où « même ssh passait à peine »
  Les boucles infinies dans du code de bibliothèque sont particulièrement désagréables
- C’est bien mieux. Il suffit d’utiliser une mémoire constante
  On a la garantie que le nombre de nœuds est inférieur ou égal à la hauteur de l’arbre
Les exceptions dans les threads sont vraiment redoutables
Il y a un récit de débogage cauchemardesque avec C++, select() et un thread qui lance des exceptions dans tous les sens : https://news.ycombinator.com/item?id=42532979
- Je me souviens avoir lu ce billet, mais je n’avais pas les connaissances du domaine nécessaires pour le comprendre
  Il va falloir que je le relise

3 200 % d’utilisation CPU

Symptômes de l’incident et premier indice

Le code soupçonné au départ et les hypothèses écartées

La boucle infinie créée par une TreeMap non protégée

Expérience de reproduction simple

Vérification du cycle interne dans TreeMap

Cas similaires existants et différences

Reproduction plus réaliste avec ExecutorService

Même problème dans un service gRPC

Hypothèses sur la cause et expériences par langage

Correction simple et défense au niveau de la structure de données

Empiler plusieurs couches de défense

Alertes NPE

Alertes sur les anomalies d’utilisation CPU

Gestion des exceptions dans l’executor

Revue de code et analyse statique

Tests

Conclusion

À lire aussi

1 commentaires

Avis sur Hacker News

3 200 % d’utilisation CPU