Hab grad einen Concurrency-Bug gefixt, den ich recht unscheinbar fand, und wollte mal wissen ob ihr den direkt „sehen“ könnt
public class TestService {
// just some other service that creates/runs a task and returns its future
private final Supplier<CompletableFuture<Void>> asyncService =
() -> CompletableFuture.runAsync(() -> {/* stuff */});
private final Map<String, CompletableFuture<Void>> runningTasks = new HashMap<>();
public synchronized void doSomethingAsync(String id) {
final CompletableFuture<Void> task = asyncService.get();
if (!task.isDone()) {
runningTasks.put(id, task);
task.thenRun(() -> {
runningTasks.remove(id);
});
}
}
public synchronized CompletableFuture<Void> afterAllTasksDone() {
final CompletableFuture<?>[] allDone = (CompletableFuture<?>[]) runningTasks.values().toArray();
return CompletableFuture.allOf(allDone);
}
}
Der benutzte Service „asyncService“ erstellt und startet tasks in einem threadpool, und kann als bugfrei angenommen werden, ebenso wie die verwendeten Java-Libs (CompletableFuture etc.).
Ich verstehe den Aufbau des Programmes überhaupt nicht, was ist denn der Sinn der ganzen Soße? Habe aber auch noch nie mit CompletableFuture gearbeitet.
Mal meine Einschätzung nach kurzem darüber schauen und dem Versuch den Code zu verstehen:
Nachdem !task.isDone() “true” zurückgibt ist der tatsächliche state floating. Sprich noch bevor task.thenRun() aufgerufen wird, kann der Task bereits beendet worden sein und der Eintrag aus der HashMap wird nie mehr gelöscht.
Zudem kann es vorkommen, dass sich eines der CompletableFuture aus der HashMap löscht, während doSomething versucht einen neuen Task hinein zu schreiben. Weiteres Concurrency Problem.
Gleiches gilt in der afterAllTasksDone Methode wenn die Einträge abgefragt werden.
Da du von einem Concurrency-Bug sprichst tippe ich drauf, das versucht wird ein Task zu entfernen - welcher noch nicht in der Map gelandet ist.
Denn selbst wenn thenRun niemals rennt - laut Doku ist das kein Problem für CompletableFuture.allOf. (Also bleibt für mich nur noch die nicht threadsafe Map).
Falls du mal mit Promises gearbeitet hast (z.B. in Javascript), genau das ist CompletableFuture: Es enthält das Ergebnis eines Tasks, sobald jener Task ansynchron (z.B. durch einen Thread-Pool) ein Ergebnis berechnet hat. Und man kann weitere Berechnungen mit thenRun dranhängen, die dann ebenfalls asynchron ausgeführt werden, sobald die vorherige Future fertig ist und ein Ergebnis zurückgibt.
Man kann also z.B. im Hauptthread mit anderen Dingen weitermachen und immer mal nachschauen ob es schon soweit ist. Die Tasks können auch selber neue Tasks generieren (quasi rekursiv), die auch wieder von irgendeinem Worker-Thread ausgeführt werden (= fork) oder auf die Ergebnisse mehrerer anderer Tasks warten (=join), weshalb man diesen ThreadPool dann ForkJoinPool nennt.
.
Das sollte denke ich kein Problem sein. Ein CompletableFuture wird nicht dadurch „ungültig“, dass es schon fertig ist, d.h. thenRun kann man auch auf ein fertiges aufrufen.
Genau. Das Problem für mich war hier das „synchronized“ Keyword in Kombination mit einem unauffälligen Lambda. Der Code sieht so aus, als ob alle Operationen, die den State der Hashmap ändern, innerhalb eines „synchronized“ Bereiches liegen und daher ja thread-sicher sein sollten.
Stimmt aber nicht, weil das lamda „runningTasks.remove(id);“ aus doSomethingAsync ja von einem ganz anderen Thread (einem aus dem Threadpool) ausgeführt wird. Sprich, das remove ist nicht synchronized, und kann jederzeit passieren, egal ob ein Lock auf „this“ vorliegt oder nicht.
Das sollte kein Problem sein, da HashMap.remove(id) einfach null zurück gibt, wenn etwas nicht vorhanden ist. Was ja hier nicht stört. Der Code ist schlicht nicht ordentlich synchronisiert, und hat in meinem Fall zum Glück eine ConcurrentModificationException geworfen : )
Ohne detailliert in den Code zu schauen hätte ich sofort auf die Map getippt, weil aus mehreren Threads darauf zugegriffen wird und die Map nicht threadsicher ist.
Besser wäre es, einfach eine ConcurrentHashMap als Implementierung zu verwenden, dann kann das Problem garnicht auftreten. Das synchronized an den zwei Methoden kannst du dir dann ganz sparen.
Was aber auch fehlerträchtig aussieht, ist die Methode doSomethingAsync. Die ID wird der Methode übergeben, der Service kommt aber vom Supplier. Wie wird sichergestellt, dass eine ID nicht doppelt vergeben wird und ein Task dadurch „verloren“ geht? Wenn die ID nur für die Löschung erforderlich sein sollte, könnte sie innerhalb der Methode generiert werden (oder ggf. durch einen anderen Supplier). Alternativ könnte man auch eine Liste verwenden, sofern die Performance das zulässt.
Oder gar nichts: Im geposteten Code ist nicht erkennbar, wofür runningTasksüberhaupt verwendet wird Aber eine synchronizedMap sollte es ja schon tun. Eigentlich muss es nicht mal eine ConcurrentHashMap sein - da gibt es auch einige Subtilitäten. Da remove ja ziemlich „sicher“ und „zustandsarm“ ist, ist das vielleicht gar nicht nötig.
Ja klar man kann auch ne ConcurrentHashMap benutzen. Ich denke mal, auch wegen dieses Bugs, je weniger händische Synchronisation man machen muss, umso besser.
Vorher will ich mich aber erstmal generell mehr mit Thread-sicheren Datenstrukturen beschäftigen, ihrer Performance, usw… Kommt ich auch so vor, als ob die JRE in dieser Hinsicht ein bisschen mau ausgestattet ist. Evtl. gibts da ja Bibliotheken mit besseren Alternativen.
Achja: Das Beispiel hier wurde eben von echtem Code auf etwas verstehbares zusammengekürzt, deswegen ist es vlt. etwas komisch
3 von 4 Leuten haben sofort erkannt, dass es an der Threadsafety der HashMap liegt.
Vielleicht ein Wink mit dem Zaunpfahl.
Das sticht einem sofort ins Auge. Aber ich mache dir keinen Vorwurf, Multithreading ist ein heikles und kompliziertes Thema.
Schau dir mal den Source von ConcurrentHashMap oder anderen multithread-sicheren Containern an: Du wirst dort keine synchronized-keywords drin finden. Das gilt für viele Concurrent Funktionalitäten im JDK.
Das synchronized keyword ist eigentlich nichts anderes als eine weiße Flagge mit der du dich dem Multithreading ergibst. Synchronized heißt: Ich verwende mehrere Threads, aber mach jetzt doch alles Singlethreaded.
Das ganze lock acquiring kostet zudem messbare Performance und kann zu Deadlocks führen.
Entweder du multithreadest oder lässt es bleiben.
Das kam jetzt hoffentlich nicht hochnäsig herüber. Ich weiß man kann das Projekt anhand des bereitgestellten Schnipsels nicht einfach mal in seiner Gesammtheit über einen Kamm ziehen. Das wäre unfair.
Das Problem hatte ja nix mit der Hashmap an sich zu tun. Mir sind/waren ConcurrentHashMaps bekannt, und ich benutzte die auch (Meine Aussage, das ich mich „vorher“ erstmal schlaumachen will, ist wohl missverständlich. Mit „vorher“ meine ich, bevor ich ein größeres Refactoring des Multithreadings anstrebe, falls ich dazu überhapt mal komme).
Es geht ja um einen Bug in bestehendem Code, nicht um „wenn man das alles anders machen würde, wär auch der Bug nicht da“, was offensichtlich sein sollte. Das ist ja nun wirklich kein großes Wunder, dass ohne Synchronisation keine Synchronisations-Bugs auftreten. Einfach den Code mal Code sein lassen
Hochnäßig nicht, aber das ist alles nicht neu für mich. Allerdings, ohne jetzt die Details der Concurrent-Hashmap genau zu kennen, wird auch die ConcurrentHashMap eben in bestimmten Situationen besser oder schlechter geeignet sein als eine anders funktierende Implemntierung.
Wie viele concurrent Reads verkraftet die? Writes? Wo sind die Schwächen? Jede Collection-Implementierung hat irgendwo ihre Schwächen. Und bevor ich eine Implementierung großflächig einsetze (also z.B. überall in meinem Code die Hashmaps durch ConcurrentHashMaps austausche), will ich über diese Schwächen, und über Alternative, genauer Bescheid wissen als ich das jetzt tue.
Es gibt ja da z.B. die Collections.synchronizedList() Methoden u.ä. die anscheinend nix anders machen, als ne normale Collection zu wrappen und die Methoden zu synchronisieren.
Das stimmt so nicht. Du scheinst davon auszugehen, dass man in einer Multithreaded-Umgebung arbeitet, weil man das unbedingt will und die maximale Performance haben will. Das muss aber nicht so sein. Wenn man z.B. Server-Anwendungen schreibt (das Code-Beispiel stammt aus einer), werden normalerweise die Requests von unterschiedlichen Threads angenommen.
Dann schreibt man zwar multithreaded Code, aber die Synchronisations-Performance-Einbusen können möglicherweise völlig egal sein. Da geht es dann vielleicht nur darum, das der Kram einfach korrekt funktioniert. Insofern ist die Aussage
Aber eine 