10 trucs à éviter en Java

Rangez votre crème solaire et vos tongues : c'est la fin de l'été !
Après une pause estivale, il est normal de tout oublier, aussi, une piqûre de rappel sur quelques mauvaises pratiques en Java ne peut pas faire de mal. Et puis c'est moins désagréable qu'une piqûre de moustique.

Je livre ici 10 trucs à éviter en vrac ; il n'y a pas de classement du pire truc à faire, ni du plus dangereux, ni du plus important, il s'agit juste un choix arbitraire de pratiques à éviter ou carrément à proscrire. Evidemment, au delà de ces 10 là, il y en a des dizaines d'autres...

1-Synchroniser sur une String, c'est mal

On ne peut pas synchroniser un bloc de code ou une méthode directement sur un String littéral en Java :

String id = "une chaîne";
...
synchronized (id) {
    ...
}

...ni même avec cette variante :

String id = "une chaîne".intern();

On ne peut pas synchroniser des Strings sans risque de deadlock, le problème étant que les Strings ne sont pas des objets comme les autres, et ceci en revanche aurait fonctionné :

Object id = new Object();
...
synchronized (id) {
    ...
}

Mais pas avec les Strings, car la JVM peut prendre la liberté de donner une instance de String qui existe déjà, on ne peut pas faire confiance à String s= "..."; dans l'espoir d'obtenir une instance spécifique, comme le confirme la specification Java à ce sujet (section 3.10.5, Java Language Spec 2.0) : "Literal strings within different classes in different packages likewise represent references to the same String object." Pour synchroniser des Strings, il faut :

• recourir à un wrappeur qui ne verrouille pas directement les Strings
• gérer la portée des String qui sont susceptibles d'être identiques (en tant qu'objet) de ceux qui doivent être considérés comme différents.

Le dernier point est tout aussi important : car dans vos programmes, en utilisant des chaînes que vous utiliseriez à mauvais escient dans un bloc de synchronisation, rien ne dit que dans les dizaines de librairies que vous utilisez, caché quelque part, il n'y a pas une ligne de code qui utiliserait la même chaîne dans un bloc de synchronisation.
Pour assurer l'exclusion mutuelle requise, il faudrait écrire quelque chose comme ça :

MutextContext mutexContext = new MutextContext();
...
String id = "une chaîne";
...
Mutex mutex = mutexContext.getMutex(id);
synchronized (mutex) {
    ...
}

Le code des classes Mutex et MutexContext est disponible dans Github. Les aficionados apprécierons dans la classe MutexContext l'usage du StampedLock apparu avec Java 8.

Avec cette implémentation, il est inutile d'être précautionneux avec les instances de Mutex créées : le contexte nettoie automatiquement celles qui ne sont plus référencées.

Ces classes ne sont cependant pas magiques :

• comme dit plus haut, c'est au programmeur de définir un contexte pas domaine afin d'éviter les collisions d'ID. La portée de tels domaines dépend de l'application
• elles ne permettent pas d'éviter le dead-lock, elles évitent simplement la possibilité de dead-lock inhérente à l'utilisation directe des String

En bonus, la classe MutexContext permet d'obtenir un verrou sous les 3 formes disponibles : Lock, ReadWriteLock, et le StampedLock évoqué précédemment, c'est à dire le verrou associé à une String dans un contexte donné.

2-Créer un singleton avec double-lock

L'enjeu est de fournir un singleton qui ne serait construit que s'il est demandé, car sa construction est coûteuse ; très simplement, on peut utiliser le code suivant qui fait l'affaire :

public class Tools {
    private Tools() {
        // a very costly constructor
    }

    private Tools singleton;

    public static synchronized Tools getInstance() {
        if (singleton == null) {
            singleton = new Tools();
        }
        return singleton;
    }
}

Le problème est que ce code est synchronisé sur chaque appel, alors qu'en définitive, seule la création du singleton requiert une synchronisation. Déjà, obtenir un verrou en Java est réputé coûteux en CPU, mais en plus faire attendre tous les threads concurrents à chaque fois peut vraiment plomber les performances. Alors améliorons notre code :

    public static Tools getInstance() {
        if (singleton == null) {
            synchronized (Tools.class) {
                if (singleton == null) {
                    singleton = new Tools();
                }
            }
        }
        return singleton;
    }

Cet idiome s'appelle le "double-check locking" et consiste à n'acquérir le verrou que si nécessaire (si le singleton n'est pas encore initialisé) : le premier test est une stratégie optimiste de vérification de la présence du singleton ; si le singleton est déjà créé, on le retourne ; dans le cas contraire, on acquiert un verrou pour que le singleton ne soit pas construit plusieurs fois par des threads concurrents.

Au premier abord, ce code fonctionne bien. Au deuxième abord, pas du tout, car cet idiome est incompatible avec le modèle de gestion de la mémoire de la JVM, qui permet ce qui est connu sous la dénomination de "out-of-order writes", ou écriture dans le désordre. La simple ligne singleton=new Tools() est en fait décomposée en 3 opérations élémentaires :

1. allocation de la mémoire
2. assignation de la référence vers le bloc mémoire alloué
3. construction de l'objet

En fait, cet ordre d'exécution dépend du compilateur JIT (Just In Time) utilisé. Et voilà le désastre ! Si un autre thread fait le premier test sur le singleton juste avant que celui-ci ne soit construit, mais juste après qu'il ait été assigné (donc : non null), l'objet retourné n'est en fait qu'une sorte d'embryon, un objet non terminé, donc non viable pour la suite des traitements. Entendons-nous bien, il ne s'agit pas d'un bug, mais bien d'un mode de gestion autorisé par la spécification de la JVM ; ce qui est sournois, c'est que le code source Java ne montre pas le défaut sous-jacent : le code Java est correct, mais il ne fait pas ce pour quoi il a été écrit.

Un article sur ce sujet détaille les mécanismes impliqués, et insiste sur le fait que toutes les variations du double-check locking sont vouées à l'échec : cette technique est définitivement non fiable.

L'idiome certainement le plus élégant pour obtenir un singleton dont la construction est coûteuse est celui du "support d'initialisation à la demande" (initialization-on-demand holder) :

public class Tools {
    private Tools() {
        // a very costly constructor
    }

    private static class SingletonHolder {
        private static final Tools TOOLS = new Tools();
    }

    public static Tools getInstance(){
        return SingletonHolder.TOOLS;
    }
}

Dans ce code, la classe SingletonHolder ne sera initialisée que lorsqu'elle sera demandée, c'est à dire exclusivement en cas d'appel de la méthode getInstance(). Il est de la responsabilité de la JVM de procéder au chargement et à l'initialisation de la classe SingletonHolder correctement, c'est à dire même si des accès concurrents surviennent. In fine, une instance est directement accessible et ne sera construite qu'au moment ou elle est demandée la première fois.

Il se trouve que ce n'est pas la seule technique permettant d'obtenir un résultat conforme : si la référence du singleton est qualifiée de "volatile", la JVM s'efforcera de maintenir une copie conforme de la référence entre tous les threads, et garantira que l'écriture ne se fera pas dans le désordre. C'est ce qui apparaît dans le code de MutexContext$LockableMutex (vers les lignes 105 et suivantes). Comme le montre ce code, cette technique n'est pas exclusivement dédiée aux singletons, contrairement à l'idiome du "support d'initialisation à la demande". L'usage de volatile est cependant un peu impactant sur les performances, mais pratiquement pas sur les lectures, aussi, s'il s'agit de construire un singleton, le "support d'initialisation à la demande" reste à privilégier.

3-Ecrire un benchmark sans savoir comment faire

La tentation de la simplicité est grande, pourquoi ne pas faire :

int count = 1000;
long start = System.currentTimeMillis();
for (int i = 0; i < count ; i++) {
    // truc à évaluer
}
long duration = System.currentTimeMillis() - start;
long avg = duration / count;

C'est à éviter pour un tas de raisons...

• System.currentTimeMillis() donne une valeur en milliseconde, dont la granularité dépend de l'OS, donne une mesure depuis le 1/1/1970 sujette à un ajustement ! System.nanoTime() est déjà plus approprié.
• Il se passe pleins de trucs dans la JVM avant qu'on atteigne une stabilité du code dans la JVM, comme le chargement des classes qui peuvent polluer une mesure pour peu qu'elles surviennent dans la boucle à évaluer. Ne croyez pas non plus que tout est plié dès la fin de la première itération.
• Selon la JVM, le code peut être interprété dans un premier temps, puis compilé dans un second temps (par exemple après 10000 invocations sur un serveur, et 1500 invocations autrement). Mais les lonnnnnngues méthodes avec des boucles sont susceptibles d'être compilées plus tôt (on appelle ça "on-stack-replacement").
• Il y a de nombreux effets de bords qui peuvent affecter les performances. Par exemple :

  void testSyncInner() {
      synchronized(this) {
          i++; // run in 8,244,087 usec
      }
  }
  synchronized void testSyncOuter() {
      i++; // run in 13,383,707 usec
  }
  

  Qui l'aurait cru ?
• La JVM sait très bien éliminer le code mort, même si le code est susceptible d'être exécuté mais le résultat non utilisé : il faut toujours consommer les résultats, mais pas les accumuler dans les champs d'une classe qui ne seront pas utilisés (la JVM sait ce que vous faites !). Par exemple :

  double n = 10;
  void stub() {}                     // 1.008
  void dead() {} {                   // 1.017
      double r = n * Math.log(n) / 2;
  }
  void alive() {} {                  // 48.514
      double r = n * Math.log(n) / 2;
      if (r==n && r==0) {
          throw new IllegalStateException();
      }
  }
  

Utilisez donc un outil approprié qui vous épargnera des métriques erronées et des conclusions fausses. Mais sachez également ce que vous faites, car l'outil ne fera pas tout pour vous. Ces outils prennent en considération le "code warm-up" (itérations d'initialisation qu'il faut ignorer) en faisant tourner les mêmes données sur le même code avant de procéder aux mesures.

Parmi les prétendants, se trouvent :

• JMH
• Caliper
• Japex
• JUnitPerf

Je ne connais que les deux premiers, et je trouve JMH vraiment remarquable : facile d'utilisation, bien documenté par de nombreux examples, et prenant à son compte nombre de processus internes à la JVM. JMH utilise Maven, offre une approche basée sur les annotations pour déterminer les benchmarks (mais sans recourir à la reflexivité pendant l'exécution du benchmark), et fourni des "trous noirs" (BlackHole) pour consommer les résutats.

Un benchmark avec JMH ressemble à ça :

@GenerateMicroBenchmark(BenchmarkType.ALL)
public void arrayListForEach(BlackHole bh) {
    for (Integer i : arrayList) {
        bh.consume(i);
    }
}
@GenerateMicroBenchmark(BenchmarkType.ALL)
public void arrayListIterator(BlackHole bh) {
    Iterator it = arrayList.iterator();
    while (it.hasNext()) {
        Integer i = it.next();
        bh.consume(i);
    }
}

C'est simple ! Et vous serez en mesure de comparer quelle méthode d'itération est la plus performante sur un ArrayList !

4-hashCode() / equals() non cohérents

(et accessoirement avec compareTo())

Pour les débutants, au cas où cela vous aurait échappé, 2 objets identiques (au sens equals()) doivent avoir le même hashcode :

a.equals(b) ⇒ a.hashCode() == b.hashCode()

Oubliez cette simple règle, et vous serez amené à affronter de nombreux bugs. Par exemple, une clé qui enfreindrait cette règle et qui sert d'accès à une HashMap pourrait donner des résultats surprenants : on dépose un objet avec cette clé, on recherche aussitôt l'objet avec une clé qui normalement correspond, mais on ne retrouve pas l'objet !

Le hashcode détermine le slot de la table de hachage dans lequel stocker l'objet. Si plusieurs objets ont le même hashcode, ceux-ci sont chaînés et un test d'égalité permet de déterminer lequel retourner lors d'une recherche. Mais si la clé de recherche (égale à celle de stockage) donne un hashcode différent, alors la recherche pourra se faire dans un mauvais slot.

Pour les objets susceptibles d'être comparés, la méthode compareTo() doit retourner zéro si et seulement si equals() retourne vrai pour les mêmes arguments non-null.

5-Synchroniser sur le mauvais objet

Cela reste anecdotique, pourtant j'ai trouvé le code ci-dessous dans un programme livré par un de mes prestataires (je ne sais même plus comment je suis tombé dessus parmi les 130000 lignes de code du projet) :

synchronized (Thread.currentThread()) {
    ...
}

J'ai également retrouvé le coupable... Même un programmeur chevronné peut écrire des inepties ! J'ai mis ça sur le compte de la fatigue.

Pour ceux qui n'ont pas compris : la synchronisation garanti que deux threads (ou plus) concurrents ne peuvent pas rentrer simultanément dans une section critique. En synchronisant sur un objet toujours différent, en l'occurrence une instance propre à chaque thread (le thread courant !), on est certain que cette synchronisation est inopérante et ne sert à rien à part bouffer du CPU.

6-Utiliser des Strings pour les mots de passe

Les Strings ne sont pas des objets comme les autres :

• les String sont immutables
• les String peuvent être mis dans des pools pour réutilisation

Ces deux caractéristiques en font un candidat peu crédible pour stocker des données aussi sensibles que des mots de passe.

En stockant les mots de passe dans des String, ceux-ci resteront dans la mémoire jusqu'au passage du Garbage Collector, ou bien plus longtemps s'ils intègrent le pool de Strings. Même après s'en être débarassé, il en restera une trace longtemps après, sans possibilité de la remplir avec des blancs (ou des zéros), ce qui représente une menace à l'égard de la sécurité. C'est pourquoi il vaut mieux utiliser des char[], car une fois utilisé, on peut les purger de leur contenu. C'est d'ailleurs recommandé dans le guide "Java Cryptography Architecture :

It would seem logical to collect and store the password in an object of type java.lang.String. However, here's the caveat: Objects of type String are immutable, i.e., there are no methods defined that allow you to change (overwrite) or zero out the contents of a String after usage. This feature makes String objects unsuitable for storing security sensitive information such as user passwords. You should always collect and store security sensitive information in a char array instead.

Et de fait, ceci est appliqué par exemple dans JPasswordField : la méthode getPassword() retourne bien un char[].

Malheureusement, ce n'est pas appliqué partout : pour ceux qui font du Web avec des servlets, il n'y a pas moyen de récupérer les saisies de formulaires autrement que sous la forme de String, ce qui est regrettable pour les mots de passe. Il serait ô combien précieux de pouvoir disposer d'une méthode qui lise directement le buffer d'entrée des caractères émis par le client pour les stocker dans un char[], tout en prenant soin de purger la source (d'autant que les classes élémentaires basées sur java.io.Reader permettent justement de lire des char[] !). Libre ensuite au développeur d'en faire bon usage puis à son tour de purger son char[].

Pour pousser la parano plus loin, on peu craindre que dans certains usages un mot de passe soit contraint de résider durablement en mémoire. En cas de vidage de la mémoire, on pourrait librement consulter la zone ou est stocké le mot de passe ; enfin... pour peu qu'on ait accès à la machine sur laquelle notre application fonctionne, mais j'ai averti qu'on était bien en mode "parano".

Il n'y a pas de solution à ce problème : si l'application doit avoir accès à un mot de passe à un moment puis à un autre, on est bien obligé de le conserver. Oui, mais entre ces deux moments, rien n'oblige à ce qu'il soit facilement accessible. Une solution qui réduit drastiquement la surface vulnérable consiste, entre ces deux moments, à rendre ce mot de passe difficilement lisible. Au lieu de le laisser en clair, on peut le base64 encoder, ou mieux, le crypter. Il faut ensuite prendre soin dans son code de réduire à son minimum le moment où le mot de passe apparaît en clair dans la mémoire.

La librairie "Alternet Security" fait ça très bien :

• Exemple et documentation
• Le code source

Si vous êtes -comme moi- complètement parano, utilisez la forme cryptée : il sera bien plus dur à un admin indélicat et peu digne de confiance de retrouver la clé, retrouver le vecteur d'initialisation, puis retrouver le mot de passe crypté, avant de lancer le bon algo de décryptage.

7-Comparer des URLs ou les utiliser dans des collections

Les URLs (java.net.URL) ne sont pas des objets comme les autres. Vous risquez d'avoir de belles (désagréables) surprises s'il vous venait à l'idée de les utiliser par exemple dans un Map ou un Set : les performances de votre application s'en trouveraient immédiatement dégradées, cela en raison des méthodes equals() et hashCode() de java.net.URL qui recourent à la résolution du nom de domaine, résolution qui dépend du bon vouloir de la connexion réseau.

Par ailleurs, l'implementation de equals() dans java.net.URL est basé sur la règle amusante que deux domaines sont identiques si les adresses IP correspondantes sont identiques, ce qui est juste totalement incompatible avec l'hébergement virtuel (virtual hosting), qui consiste à regrouper sur une adresse IP plusieurs noms de domaines.

Utilisez à la place java.net.URI, qui est -mais pas que- la version inerte de l'URL. Une instance de URI peut s'obtenir immédiatement depuis une URL avec la méthode toURI().

8-Obtenir un nombre aléatoire

Ce code est amusant ; c'est davantage un trait d'humour qu'un vrai cas à éviter.

int getRandomNumber() {
    return 4; // chosen by fair dice roll.
              // guaranteed to be random
}

9-Produire de la documentation inutile

Java nous habitue -voire nous oblige- à documenter nos classes, ce qui est bien en soit, mais pas comme ça :

    /**
    * A constructor. Takes no parameters
    * and creates a new instance of MyClass.
    */
    public MyClass() {}

Il est vraiment inutile de documenter ce que fait le langage Java :

    index++; // increment index

Contentez vous de documenter ce que vous faites (dans le code) et comment utiliser vos classes et vos méthodes.

Il est vrai aussi qu'on est souvent obligé de créer de la documentation pour un tel constructeur. S'il y a un comportement par défaut et/ou des valeurs implicites, on peut les décrire dans la documentation, sinon on peut se contenter du minimum.

10-Gérer du XML comme du String

Je rencontre encore assez souvent du code qui ressemble à ça :

String title = ...
String code = ...
String xml = "<books>"
            +"<book code=\""+ code +"\">"+ title +"</book>"
            +"</books>";

...voire avec du System.out.println(), ce qui n'est pas mieux et autant pire.

Ce code est peu robuste, il suffit que le titre de notre livre soit "Le rouge & le noir" et le XML produit sera erroné, impossible à traiter par une application.

Il y a 1001 façons de produire correctement du XML, chacune adaptée au problème à traiter, mais celles à éviter sont celles qui reposent sur le traitement des balises en tant que String. On peut par exemple, créer un DOM, et construire l'arborescence de la structure cible puis sérialiser le résultat en XML. Pénible, je l'admet, mais correct au sens XML. Si on veut directement produire un flot de caractères XML, on peut recourir plus simplement à XMLStreamWriter :

String title = ...
String code = ...
XMLStreamWriter xml = ...
xml.writeStartDocument();
xml.writeStartElement("books");
xml.writeStartElement("book");
xml.writeAttribute("code", code);
xml.writeCharacters(title);
xml.writeEndElement();
xml.writeEndElement();
xml.writeEndDocument();

...un peu plus verbeux mais pas moins lisible, et beaucoup plus efficace. Pour finir, si vous disposez d'une hiérarchie d'objets à transposer en XML, JAXB est fait pour vous.

Et si vous aviez la mauvaise idée d'analyser directement du texte sans recourir à un parseur XML, vous êtes dans l'erreur :

int start = xml.indexOf("<book>") + "<book>".length();
int end = xml.indexOf("</book>");
String title = xml.substring(start, end);

...car si dans mon source XML se trouve ceci :

    <!-- <book>Le rouge &amp; le noir </book> -->

...ou ceci :

    <![CDATA[<book>Le rouge &amp; le noir </book>]]>

...l'extraction d'un <book> donnera un résultat erroné, puisque dans le premier cas il s'agit d'un commentaire, et dans le second d'un littéral (le balisage n'en est pas un).

Combien même la bonne chaîne de caractère serait incluse dans le bon élément, le résultat de notre livre serait : "Le rouge & le noir" ! Faites un essai chez votre libraire :

-Bonjour monsieur, auriez-vous "le rouge et amp point virgule le noir" ?

Rappelons-le : XML a beau être un format de texte, il faut utiliser les outils adaptés pour le traiter correctement, c'est à dire prendre en compte toutes les subtilités du langage, comme par exemple la substitution des appels d'entités ou la prise en charge de l'encodage. Un traitement hâtif qui repose sur des Strings ou même des expressions régulières est voué à l'échec.

Conclusion

Le lecteur attentif aura remarqué qu'il a été souvent question de String dans cet article. Alors je le répète : comme les vacances sont terminées, laissez vos String dans les placards !

Semantic Mismatch
ERR-06 : ArgumentNumberException
SABOT accept only a single PIED
-- See log for details