L'utilisation d'une carte de cache est-elle sûre dans un environnement multithread?

Question

J'implémente un cache LRU pour les photos des utilisateurs, en utilisant Commons Collections LRUMap (qui est fondamentalement un LinkedHashMap avec de petites modifications). La méthode findPhoto peut être appelée plusieurs centaines de fois en quelques secondes.

public class CacheHandler { 
    private static final int MAX_ENTRIES = 1000; 
    private static Map<Long, Photo> photoCache = Collections.synchronizedMap(new LRUMap(MAX_ENTRIES)); 

    public static Map<Long, Photo> getPhotoCache() { 
     return photoCache; 
    } 
} 

Utilisation:

public Photo findPhoto(Long userId){ 
    User user = userDAO.find(userId); 
    if (user != null) { 
     Map<Long, Photo> cache = CacheHandler.getPhotoCache(); 

     Photo photo = cache.get(userId); 
     if(photo == null){ 
      if (user.isFromAD()) { 
       try { 
        photo = LDAPService.getInstance().getPhoto(user.getLogin()); 
       } catch (LDAPSearchException e) { 
        throw new EJBException(e); 
       } 
      } else { 
       log.debug("Fetching photo from DB for external user: " + user.getLogin()); 
       UserFile file = userDAO.findUserFile(user.getPhotoId()); 
       if (file != null) { 
        photo = new Photo(file.getFilename(), "image/png", file.getFileData()); 
       } 
      } 
      cache.put(userId, photo); 
     }else{ 
      log.debug("Fetching photo from cache, user: " + user.getLogin()); 
     } 
     return photo; 

    }else{ 
     return null; 
    } 
} 

Comme vous pouvez le voir, je ne suis pas en utilisant des blocs de synchronisation. Je suppose que le cas le plus défavorable est une condition de concurrence qui entraîne l'exécution de deux threads par cache.put (userId, photo) pour le même userId. Mais les données seront les mêmes pour deux threads, donc ce n'est pas un problème.

Est-ce que mon raisonnement est correct? Si ce n'est pas le cas, existe-t-il un moyen d'utiliser un bloc de synchronisation sans avoir un impact important sur les performances? Avoir seulement 1 fil accédant à la carte à la fois se sent comme exagéré.

Answer 1

Oui, vous avez raison - si la création photo est idempotente (renvoie toujours la même photo), la pire chose qui puisse arriver est que vous la récupérerez plus d'une fois et la mettra plus d'une fois sur la carte.

Answer 2

Assylias a raison de dire que ce que vous avez fonctionnera bien. Cependant, si vous voulez éviter d'aller chercher des images plus d'une fois, c'est aussi possible, avec un peu plus de travail. L'idée est que si un thread arrive, fait manquer un cache, et commence à charger une image, alors si un deuxième thread veut la même image avant que le premier thread ait fini de le charger, alors il devrait attendre le premier thread, plutôt que d'aller le charger lui-même.

Il est assez facile à coordonner en utilisant certaines des classes de concurrence les plus simples de Java. D'abord, permettez-moi de refactoriser votre exemple pour sortir le peu intéressant. Voici ce que vous avez écrit:

public Photo findPhoto(User user) { 
    Map<Long, Photo> cache = CacheHandler.getPhotoCache(); 

    Photo photo = cache.get(user.getId()); 
    if (photo == null) { 
     photo = loadPhoto(user); 
     cache.put(user.getId(), photo); 
    } 
    return photo; 
} 

Ici, loadPhoto est une méthode qui fait la débrouille réelle de chargement d'une photo, ce qui est pertinent en l'espèce. Je suppose que la validation de l'utilisateur est faite dans une autre méthode qui appelle celle-ci. A part ça, c'est votre code.

Ce que nous faisons est plutôt ceci:

public Photo findPhoto(final User user) throws InterruptedException, ExecutionException { 
    Map<Long, Future<Photo>> cache = CacheHandler.getPhotoCache(); 

    Future<Photo> photo; 
    FutureTask<Photo> task; 

    synchronized (cache) { 
     photo = cache.get(user.getId()); 
     if (photo == null) { 
      task = new FutureTask<Photo>(new Callable<Photo>() { 
       @Override 
       public Photo call() throws Exception { 
        return loadPhoto(user); 
       } 
      }); 
      photo = task; 
      cache.put(user.getId(), photo); 
     } 
     else { 
      task = null; 
     } 
    } 

    if (task != null) task.run(); 

    return photo.get(); 
} 

Notez que vous devez changer le type de CacheHandler.photoCache pour accueillir les FutureTask s emballage. Et puisque ce code fait un verrouillage explicite, vous pouvez en retirer le synchronizedMap. Vous pouvez également utiliser un ConcurrentMap pour le cache, ce qui permettrait l'utilisation de putIfAbsent, une alternative plus concurrente à la séquence lock/get/check pour la séquence null/put/unlock.

Espérons que ce qui se passe ici est assez évident. Le schéma de base consistant à récupérer quelque chose dans le cache, à vérifier si ce que vous avez obtenu était nul, et si c'est le cas, il y a toujours quelque chose. Mais au lieu de mettre dans Photo, vous mettez dans un Future, qui est essentiellement un espace réservé pour un Photo qui ne peut pas (ou peut) être là juste à ce moment, mais qui sera disponible plus tard. La méthode get sur Future obtient la chose pour laquelle une place est retenue, bloquant jusqu'à ce qu'elle arrive si nécessaire.Ce code utilise FutureTask comme implémentation de Future; ceci prend un Callable capable de produire un Photo en tant qu'argument de constructeur, et l'appelle quand sa méthode run est appelée. L'appel à run est protégé par un test qui récapitule essentiellement le test if (photo == null) précédemment, mais en dehors du bloc synchronized (car comme vous l'avez compris, vous ne voulez vraiment pas charger les photos en maintenant le verrou du cache).

Ceci est un motif que j'ai vu ou besoin de quelques fois. C'est dommage qu'il ne soit pas intégré dans la bibliothèque standard quelque part.