Bloque sincronizado de Java frente a Collections.synchronizedMap

¿Está configurado el siguiente código para sincronizar correctamente las llamadas synchronizedMap?

public class MyClass {
  private static Map<String, List<String>> synchronizedMap = Collections.synchronizedMap(new HashMap<String, List<String>>());

  public void doWork(String key) {
    List<String> values = null;
    while ((values = synchronizedMap.remove(key)) != null) {
      //do something with values
    }
  }

  public static void addToMap(String key, String value) {
    synchronized (synchronizedMap) {
      if (synchronizedMap.containsKey(key)) {
        synchronizedMap.get(key).add(value);
      }
      else {
        List<String> valuesList = new ArrayList<String>();
        valuesList.add(value);
        synchronizedMap.put(key, valuesList);
      }
    }
  }
}

Según tengo entendido, necesito el bloque sincronizado addToMap()para evitar que otro subproceso llame remove()o containsKey()antes de realizar la llamada a, put()pero no necesito un bloque sincronizado doWork()porque otro subproceso no puede ingresar al bloque sincronizado addToMap()antes de las remove()devoluciones porque creé el mapa originalmente con Collections.synchronizedMap(). ¿Es eso correcto? ¿Hay una mejor manera de hacer esto?

Collections.synchronizedMap() garantiza que se sincronizará cada operación atómica que desee ejecutar en el mapa.

Sin embargo, la ejecución de dos (o más) operaciones en el mapa debe estar sincronizada en un bloque. Entonces sí, estás sincronizando correctamente.

Si está utilizando JDK 6, es posible que desee consultar ConcurrentHashMap

Tenga en cuenta el método putIfAbsent en esa clase.

Existe la posibilidad de un error sutil en su código.

[ ACTUALIZACIÓN: Ya que está usando map.remove () esta descripción no es totalmente válida. Me perdí ese hecho la primera vez. :( Gracias al autor de la pregunta por señalar eso. Dejo el resto como está, pero cambié la declaración principal para decir que hay un error potencial .]

En doWork () obtienes el valor de Lista del Mapa de una manera segura para subprocesos. Después, sin embargo, está accediendo a esa lista en un asunto inseguro. Por ejemplo, un hilo puede estar usando la lista en doWork () mientras que otro hilo invoca synchronizedMap.get (key) .add (value) en addToMap () . Esos dos accesos no están sincronizados. La regla general es que las garantías de seguridad para subprocesos de una colección no se extienden a las claves o valores que almacenan.

Puede solucionar este problema insertando una lista sincronizada en el mapa como

List<String> valuesList = new ArrayList<String>();
valuesList.add(value);
synchronizedMap.put(key, Collections.synchronizedList(valuesList)); // sync'd list

Alternativamente, puede sincronizar en el mapa mientras accede a la lista en doWork () :

  public void doWork(String key) {
    List<String> values = null;
    while ((values = synchronizedMap.remove(key)) != null) {
      synchronized (synchronizedMap) {
          //do something with values
      }
    }
  }

La última opción limitará un poco la concurrencia, pero es algo más clara en mi opinión.

Además, una nota rápida sobre ConcurrentHashMap. Esta es una clase realmente útil, pero no siempre es un reemplazo apropiado para HashMaps sincronizados. Citando de sus Javadocs,

Esta clase es completamente interoperable con Hashtable en programas que dependen de su seguridad de subprocesos pero no de sus detalles de sincronización .

En otras palabras, putIfAbsent () es ideal para inserciones atómicas pero no garantiza que otras partes del mapa no cambiarán durante esa llamada; garantiza solo la atomicidad. En su programa de muestra, está confiando en los detalles de sincronización de (un sincronizado) HashMap para otras cosas que no sean put ().

Última cosa. :) Esta gran cita de Java Concurrency in Practice siempre me ayuda a diseñar programas de depuración de múltiples subprocesos.

Para cada variable de estado mutable a la que puede acceder más de un hilo, todos los accesos a esa variable deben realizarse con el mismo bloqueo retenido.

Sí, estás sincronizando correctamente. Explicaré esto con más detalle. Debe sincronizar dos o más llamadas a métodos en el objeto synchronizedMap solo en el caso de que tenga que confiar en los resultados de llamadas a métodos anteriores en la siguiente llamada al método en la secuencia de llamadas a métodos en el objeto synchronizedMap. Echemos un vistazo a este código:

synchronized (synchronizedMap) {
    if (synchronizedMap.containsKey(key)) {
        synchronizedMap.get(key).add(value);
    }
    else {
        List<String> valuesList = new ArrayList<String>();
        valuesList.add(value);
        synchronizedMap.put(key, valuesList);
    }
}

En este código

synchronizedMap.get(key).add(value);

y

synchronizedMap.put(key, valuesList);

las llamadas a métodos se basan en el resultado de la anterior

synchronizedMap.containsKey(key)

llamada al método.

Si la secuencia de llamadas al método no se sincronizó, el resultado podría ser incorrecto. Por ejemplo, thread 1está ejecutando el método addToMap()y thread 2está ejecutando el método doWork() La secuencia de llamadas al método en el synchronizedMapobjeto podría ser la siguiente: Thread 1ha ejecutado el método

synchronizedMap.containsKey(key)

y el resultado es " true". Después de que el sistema operativo haya cambiado el control de ejecución thread 2y haya ejecutado

synchronizedMap.remove(key)

Después de que el control de ejecución se haya cambiado de nuevo a thread 1y se haya ejecutado, por ejemplo

synchronizedMap.get(key).add(value);

creyendo que el synchronizedMapobjeto contiene el keyy se NullPointerExceptionlanzará porque synchronizedMap.get(key) regresará null. Si la secuencia de llamadas al método en el synchronizedMapobjeto no depende de los resultados de cada uno, no es necesario sincronizar la secuencia. Por ejemplo, no es necesario sincronizar esta secuencia:

synchronizedMap.put(key1, valuesList1);
synchronizedMap.put(key2, valuesList2);

aquí

synchronizedMap.put(key2, valuesList2);

La llamada al método no se basa en los resultados de la

synchronizedMap.put(key1, valuesList1);

llamada al método (no le importa si algún hilo ha interferido entre las dos llamadas al método y, por ejemplo, ha eliminado el key1).

Eso me parece correcto. Si tuviera que cambiar algo, dejaría de usar Collections.synchronizedMap () y sincronizaría todo de la misma manera, solo para que quede más claro.

Además, reemplazaría

  if (synchronizedMap.containsKey(key)) {
    synchronizedMap.get(key).add(value);
  }
  else {
    List<String> valuesList = new ArrayList<String>();
    valuesList.add(value);
    synchronizedMap.put(key, valuesList);
  }

con

List<String> valuesList = synchronziedMap.get(key);
if (valuesList == null)
{
  valuesList = new ArrayList<String>();
  synchronziedMap.put(key, valuesList);
}
valuesList.add(value);

La forma en que ha sincronizado es correcta. Pero hay una trampa

1. El contenedor sincronizado proporcionado por el marco de la colección asegura que las llamadas al método, es decir, agregar / obtener / contener, se ejecutarán mutuamente.

Sin embargo, en el mundo real, generalmente consultaría el mapa antes de ingresar el valor. Por lo tanto, necesitaría realizar dos operaciones y, por lo tanto, se necesita un bloque sincronizado. Entonces, la forma en que lo ha usado es correcta. Sin embargo.

2. Podría haber utilizado una implementación simultánea de Map disponible en el marco de la colección. El beneficio 'ConcurrentHashMap' es

a. Tiene una API 'putIfAbsent' que haría lo mismo pero de una manera más eficiente.

segundo. Es eficiente: dThe CocurrentMap simplemente bloquea las teclas, por lo que no bloquea todo el mundo del mapa. Donde ya ha bloqueado claves y valores.

C. Podría haber pasado la referencia de su objeto de mapa en otro lugar de su base de código donde usted / otro desarrollador en su tean puede terminar usándolo incorrectamente. Es decir, puede que todos agreguen () u obtengan () sin bloquear el objeto del mapa. Por lo tanto, su llamada no se ejecutará de forma mutuamente exclusiva para su bloque de sincronización. Pero el uso de una implementación simultánea le da la tranquilidad de que nunca se puede usar / implementar incorrectamente.

Echa un vistazo a Google Colecciones ' Multimap, por ejemplo, la página 28 de esta presentación .

Si no puede usar esa biblioteca por alguna razón, considere usar en ConcurrentHashMaplugar de SynchronizedHashMap; tiene un putIfAbsent(K,V)método ingenioso con el que puede agregar atómicamente la lista de elementos si aún no está allí. Además, considere usar CopyOnWriteArrayListpara los valores del mapa si sus patrones de uso lo justifican.