Statisches vs Instanz-Synchronisieren

Neulich stolperte ich über eine Klasse, in der viele synchronisierte Methoden verwendet wurden und dabei in eine synchronized Falle getappt wurde: Statisches vs Instanz-Synchronisieren.

Eigentlich scheint das Schlüsselwort synchronized in seiner Verwendung einfach zu sein. Einfach Methoden damit versehen und sie sind sicher vor konkurrierenden, nebenläufigen Zugriffen. Das Beispiel, über das ich stolperte, sah in etwa wie folgt aus. Es handelte sich um eine Klasse, welche als Singleton konzipiert war und als Cache fungieren sollte:

public class Container {
    private static HashMap<Long, EntityDto> entityDtoCacheById = new HashMap<>();
    
    private static Container instance = null;    
    
    public static Container getInstance() {
         if (instance == null) {
            synchronized (Container.class) {
                if (instance == null) {
                    instance = new Container();
                }
            }
        }
         
        return instance;
    }
    
    public synchronized EntityDto getEntityDto(Long id) {
        EntityDto entityDto = entityDtoCacheById.get(id);
        if (entityDto == null) {
            Entity entity = getEntityFromDb(id);
            if (entity == null) {
                return null;
            }
            entityDto = new EntityDto(entity);
            entityDtoCacheById.put(id, entityDto);
        }
        return entityDto;
    }
    
    public static synchronized void invalidateCachedEntity(Long id) {
        EntityDto bean = entityDtoCacheById.remove(id);
        LOG.debug(bean +" wurde aus dem DBCache entfernt");
    }

    public static void incrementEntityDtoValue(Long id) {
        EntityDto entityDto = entityDtoCacheById.get(id)
        entityDto.increment();
    }

    // ...
}

Im Prinzip wurden hier viele Fehler, die zur Threadsicherheit gemacht werden können, auch gemacht. Generell hat man hier statische und Instanzmethoden vermischt. Auch für Singletons gilt, dass sie alle Zustände in Instanzvariablen speichern sollten. Die Cache-Map sollte eigentlich zur Instanz gehören und die einzige statische Variable sollte instance sein.

Die Mischung von statischen und Instanzmethoden führt hier zu einem wichtigen, aber leicht zu übersehenden Problem. Das die Methode incrementEntityDtoValue nicht sicher ist, weil sie nicht synchronisiert ist, fällt sofort auf, denn alle Zugriffe auf die gemeinsame Ressource müssen abgesichert sein.

Was aber nicht auffällt ist, dass die Methoden invalidateCachedEntity und getEntityDto ebenfalls unsicher sind, obwohl sie synchronized verwenden. Warum?

Bei der Verwendung von synchronized ist es immer wichtig im Blick zu haben, auf welchem Objekt synchronisiert wird - was also die Sperre ist.

Bei der Methode getEntityDto wird die konkrete Instanz verwendet, also die Containerinstanz. Hätten wir mehrere Containerinstanzen, würde das synchronized jeweils nur die eine Instanz zum Sperren verwenden. Dies entspricht einem synchronized(this).

Doch welche Instanz sollte verwendet werden, wenn eine statische Methode synchronisiert wird? Keine. Statische Methoden synchronisieren auf der Klasse. Das entspricht einem synchronized(Container.class).

Die Methoden invalidateCachedEntity und getEntityDto verwenden also komplett verschiedene Sperren und sind dementsprechend nicht gegen Zugriffe aufeinander abgesichert.

Eine einfache Lösung für dieses Problem wäre es, auf der verwendeten Ressource zu synchronisieren, egal ob in einer statischen oder Instanzvariable:

public EntityDto getEntityDto(Long id) {
        synchronized(entityDtoCacheById){
            EntityDto entityDto = entityDtoCacheById.get(id);
            if (entityDto == null) {
                Entity entity = getEntityFromDb(id);
                if (entity == null) {
                    return null;
                }
                entityDto = new EntityDto(entity);
                entityDtoCacheById.put(id, entityDto);
            }
            return entityDto;
        }
    }
    
    public static void invalidateCachedEntity(Long id) {
        synchronized(entityDtoCacheById){
            EntityDto bean = entityDtoCacheById.remove(id);
            LOG.debug(bean +" wurde aus dem DBCache entfernt");
        }
    }

Beide Methoden synchronisieren nun auf der statischen Variable und sind somit abgesichert.

Das löst aber natürlich nicht die anderen Probleme der Klasse. Zum Beispiel habe ich eine andere Stelle im Code gefunden, welche ein new Container() aufruft und dazu führt, das in der Anwendung plötzlich zwei Instanzen herumgeistern. Fehler, die auf zwei Instanzen eines Singleton basieren, sind sehr schwer zu erkennen, da man sie nicht erwartet.

Etwas aufgeräumt sah die Klasse nach dem Refactoring wie folgt aus:

public final class Container {
    private static Container INSTANCE;
    private Map<Long, EntityDto> entityDtoCacheById = new HashMap<>();
    
    private Container() {}
    
    public static final synchronized Container getInstance() {
        if (INSTANCE == null) {
            INSTANCE = new Container();
        }
        return INSTANCE;
    }
    
    public synchronized EntityDto getEntityDto(Long id) {
        EntityDto entityDto = entityDtoCacheById.get(id);
        if (entityDto == null) {
            Entity entity = getEntityFromDb(id);
            if (entity == null) {
                return null;
            }
            entityDto = new EntityDto(entity);
            entityDtoCacheById.put(id, entityDto);
        }
        return entityDto;
    }
    
    public synchronized void invalidateCachedEntity(Long id) {
        EntityDto bean = entityDtoCacheById.remove(id);
        LOG.debug(bean +" wurde aus dem DBCache entfernt");
    }

    public synchronized void incrementEntityDtoValue(Long id) {
        EntityDto entityDto = entityDtoCacheById.get(id)
        entityDto.increment();
    }
}

Die Kombination aus final class und privatem Konstruktor soll dafür sorgen, dass neue Instanzen ohne unsere getInstance-Methode unmöglich werden - mit der final class verhindern wir zusätzlich, dass unsere Klasse überschrieben wird und damit z. B. einen neuen Konstruktor anbietet.

Alle Zustände sind nun Instanzvariablen und damit wird auch verhindert, dass ein anderer Entwickler dazu verleitet wird, neue statische Methoden zu schreiben, um den Zustand zu ändern.

Weiterhin sind alle Methoden Instanzmethoden. So könnten wir die Methoden mit synchronized verwenden und sind (größtenteils) sicher.

Diese Trennung der Zuständigkeit über den Zustand hat auch einen weiteren Vorteil: Möchten wir irgendwann unser Objekt doch nicht als Singleton verwenden, (weil wir z. B. zwei oder mehr Caches für verschiedene Entities verwenden wollen) dann sind die notwendigen Änderungen minimal: Statt per getInstance kann dann eine neue Instanz erzeugt und an die verwendeten Stellen weitergegeben werden.

Bleibt nur noch die Tatsache, das getInstance auf die Klasse, aber alle anderen Methoden auf die Instanz synchronisieren. Kann das zu einem Problem werden? Nein, denn die Synchronisierung auf die Klasse schützt die Ressource INSTANCE, während die Synchronisierungen auf die Instanz die Ressource entityDtoCacheById schützen - so haben beide unterschiedlichen Sperren einen eigenen, getrennten Verantwortungsbereich und können sich damit nicht gegenseitig stören.