[JAVA] Vraagtekens bij threadsafety

[Kwistnix]

Ik heb een eenvoudige klasse FileChangeMonitor geschreven, waarbij objecten die een FileChangeListener interface implementeren zich kunnen registreren om genotificeerd te worden van wijzingen aan een specifiek bestand. Deze klasse wil ik thread safe maken, maar ik heb hier niet bijzonder veel ervaring mee en ik vraag mij af hoe ik dit het beste kan aanpakken. Daarmee bedoel ik zo efficiënt mogelijk; zonder overbodige synchronisatie.

De relevante niet thread-safe code:

Java:

public class FileChangeMonitor {

    private static FileChangeMonitor instance = new FileChangeMonitor();
    private Map<String, Set<FileChangeListener>> fileListenerMap;
    private Map<String, FileMonitoringTask> taskMap;
    private Timer timer;
    private long pollingInterval = 3000L;
    private long initialDelay = 3000L;

    private FileChangeMonitor() {
        fileListenerMap = new HashMap<String, Set<FileChangeListener>>();
        taskMap = new HashMap<String, FileMonitoringTask>();
        timer = new Timer(true);
    }

    public static FileChangeMonitor getInstance() {
        return instance;
    }

    public void registerListener(FileChangeListener listener, String file) {
        if (listener == null || file == null) {
            throw new IllegalArgumentException(
                    "Supplied parameter may not be null.");
        } else {
            Set<FileChangeListener> listenerSet;
            if (fileListenerMap.containsKey(file)) {
                listenerSet = fileListenerMap.get(file);
                listenerSet.add(listener);
            } else {
                listenerSet = new HashSet<FileChangeListener>();
                listenerSet.add(listener);
                fileListenerMap.put(file, listenerSet);
                scheduleFileMonitoringTask(file);
            }

        }
    }

    public void removeListener(FileChangeListener listener, String file) {
        Set<FileChangeListener> listenerSet = null;

        if (fileListenerMap.containsKey(file)) {
            listenerSet = fileListenerMap.get(file);
            listenerSet.remove(listener);
            if (listenerSet.size() == 0) {
                fileListenerMap.remove(listenerSet);
                cancelFileMonitoringTask(file);
            }

        }
    }

    private void cancelFileMonitoringTask(String file) {
        FileMonitoringTask task = taskMap.get(file);
        task.cancel();
        taskMap.remove(file);
    }

    private void scheduleFileMonitoringTask(String file) {
        FileMonitoringTask task = new FileMonitoringTask(file);
        timer.scheduleAtFixedRate(task, initialDelay, pollingInterval);
        taskMap.put(file, task);
    }

    public long getPollingInterval() {
        return pollingInterval;
    }

    public void setPollingInterval(long pollingInterval) {
        this.pollingInterval = pollingInterval;
    }

    public long getInitialDelay() {
        return initialDelay;
    }

    public void setInitialDelay(long initialDelay) {
        this.initialDelay = initialDelay;
    }

    private void fireFileChangeEvent(String file) {
        Set<FileChangeListener> listenerSet = fileListenerMap.get(file);
        for (FileChangeListener listener : listenerSet) {
            listener.fileChanged();
        }
    }
}

Nogal wat code, maar aangezien ik deze klasse thread-safe wil krijgen is het m.i. alleemaal relevant.
Natuurlijk verwacht ik geen uitgekauwde oplossing en heb ik er zelf ook al over nagedacht

Hoe ik de situatie inschat:

1) De klasse is als Singleton geïmplementeerd. Deze implementatie is thread-safe, omdat er geen 'lazy loading' gebruikt wordt en de constructor volledig afgeschermd is.

2) In de registerListener methode kan het in een MT situatie fout gaan, omdat de Map die de listeners bevat in de class scope zit. Meerdere threads gebruiken allemaal per definitie dezelfde instantie van FileChangeMonitor, want --> Singleton. Bij het muteren van de Map kunnen daardoor problemen ontstaan, want de concrete imlementatie (HashMap) is van zichzelf niet thread-safe.

3) Bovenstaande geldt ook voor de removeListener methode.

4) De class scope variabelen pollingInterval en initialDelay kunnen gemuteerd worden m.b.v. standaard accessor methoden. Ook hier kan een probleem ontstaan in een MT omgeving.

Oplossingen die ik zelf zie:

1) De methoden registerListener, removeListener en de accessor methoden voor pollingInterval en initialDelay in hun geheel synchroniseren m.b.v. het synchronized keyword. Correct?

2) fileListenerMap instantiëren als threadsafe collection m.b.v. Collections.synchronizedMap(..) en binnen de methoden registerListener en removeListener een synchronized block aanbrengen, waarbij gesynchroniseerd wordt op fileListenerMap. De convinience methoden scheduleFileMonitorTask en cancelFileMonitoringTask worden aangeroepen vanuit deze synchronized context, waardoor hier niets aan te veranderd moet worden en ook taskMap niet als synchronizedMap geïnstantieerd moet worden. De class scope variabelen pollingInterval en initialDelay volatile maken.

Lijkt mij dat oplossing 2 sowieso efficiënter zal zijn, maar is thread safety op die manier gegarandeerd. Is dat überhaupt bij oplossing 1 wel het geval? Heb ik nog mogelijke problemen over het hoofd gezien?

[NetForce1]

Kijk eens naar de classes in java.util.concurrent, met name ConcurrentHashmap is hier interessant. Je zou nog kunnen overwegen om aparte monitors aan te maken voor initialDelay en pollingInterval, maar ik betwijfel of dat wat zal opleveren.

[Alarmnummer]

NetForce1 schreef op donderdag 30 november 2006 @ 13:16:
Kijk eens naar de classes in java.util.concurrent, met name ConcurrentHashmap is hier interessant.

Je zit dan nog steeds met atomic problemen:

code:

if(!blalist.contains(foo))
    blalist.add(foo);

Ook al is blalist threadsafe, deze call is (zonder extra locking) niet threadsafe

Ik zou in dit geval gewoon een copy on write toepassen (aangezien listeners toch niet heel vaak worden weggehaald, toegevoegd worden). Hiervoor moet je wel heel even een lock krijgen, maar nadat de lock is geweest, hoeft de structuur later niet meer gelocked te worden omdat het niet meer gewijzigd gaat worden (je gaat iedere keer een nieuwe structuur maken).

zie bv:
http://java.sun.com/j2se/...CopyOnWriteArrayList.html

Je zou nog kunnen overwegen om aparte monitors aan te maken voor initialDelay en pollingInterval, maar ik betwijfel of dat wat zal opleveren.

Het zal zeker visibility problemen helpen te voorkomen. Ik zou de velden volatile maken die je niet in een synchronized block wilt aanspreken, maar toch door meerdere threads worden aangesproken.

ps:
met longs (en doubles) kan je nog meer lol hebben in een multithreaded omgeving aangezien de read of een write namelijk niet atomic hoeft te zijn. Als een thread een long variable uitleest, is het mogelijk dat hij de 1e 4 bytes ziet van de nieuwe long, en de laatste 4 bytes van de oude long. Dus krijg je echte magic numbers

[edit]
@TS: Ik heb nog niet echt gekeken naar je probleem. Ik zal kijken of ik daar vanavond iets meer tijd voor heb.

[ Voor 25% gewijzigd door Alarmnummer op 30-11-2006 13:53 ]

[Kwistnix]

Alarmnummer schreef op donderdag 30 november 2006 @ 13:46:
[...]

Ik zou in dit geval gewoon een copy on write toepassen (aangezien listeners toch niet heel vaak worden weggehaald, toegevoegd worden). Hiervoor moet je wel heel even een lock krijgen, maar nadat de lock is geweest, hoeft de structuur later niet meer gelocked te worden omdat het niet meer gewijzigd gaat worden (je gaat iedere keer een nieuwe structuur maken).

Ik verwacht inderdaad niet dat er ontzettend veel gemuteerd gaat worden aan de map van geregistreerde listeners. Dus bij iedere mutatie een kopie maken van de onderliggende array (de collection clonen dus?) is misschien wel een optie, ondanks dat het een dure operatie is.
Bij het afvuren van events wordt dan wel oude data gebruikt, maar dat is geen ramp natuurlijk.
Het notificeren van de listeners loopt in ieder geval door ten tijde van mutaties, die dan ook gewoon kunnen plaatsvinden.

[Alarmnummer]

FallenAngel666 schreef op donderdag 30 november 2006 @ 23:17:
[...]


Ik verwacht inderdaad niet dat er ontzettend veel gemuteerd gaat worden aan de map van geregistreerde listeners. Dus bij iedere mutatie een kopie maken van de onderliggende array (de collection clonen dus?) is misschien wel een optie, ondanks dat het een dure operatie is.

Dingen die niet vaak gebeuren zijn niet zo duur. EN het voordeel aan deze aanpak is dat je veel minder hoeft te locken: alleen bij het toevoegen en het verwijderen, en niet bij het gebruik maken van de list (iets dat dus veel vaker zal voorkomen als het goed is).

Bij het afvuren van events wordt dan wel oude data gebruikt, maar dat is geen ramp natuurlijk.

Wat is oud

Het notificeren van de listeners loopt in ieder geval door ten tijde van mutaties, die dan ook gewoon kunnen plaatsvinden.

Precies. Zo doe ik het ook met listeners en het werkt uitstekend.