From a71f1e7e3f13eab69d05be5e9f44f5e74b18c976 Mon Sep 17 00:00:00 2001
From: Steven Barclay <stejbac@gmail.com>
Date: Mon, 6 Apr 2020 11:20:53 +0800
Subject: [PATCH] Make UserThread::runAfter* methods thread safe

The runAfter* methods delegate to UITimer::run(Later|Periodically) in
the case of the desktop application. These use the JavaFX TimeLine API
(via bisq.common.reactfx.FXTimer) to schedule future events. However,
this API isn't thread safe and isn't meant to be called outside the FX
application thread. This causes occasional misfirings and out-of-order
scheduling when UserThread::runAfter is called outside the user thread.

Make the UITimer::run* methods safe to call from any thread by checking
we are in the application thread and delegating to UserThread::execute
otherwise. This also improves consistency between the contracts of the
runAfter* and execute methods. As the former has many call sites, this
is safer than trying to track down all the non-thread-safe uses.

(The Timer used in the headless app already appears to be thread-safe.)

This fixes #4055 (Bisq sometimes fails to prompt user for password to
unlock wallet), caused by out-of-order scheduling of the execute and
runAfter tasks in the WalletConfig.onSetupCompleted anonymous class
method in bisq.core.btc.setup.WalletsSetup.initialize.

Also prevent an exception caused by non-thread-safe calls into JavaFX
during the shutdown of OpenOfferManager, which was uncovered by the
above, by adding a missing UserThread::execute call.
---
 .../bisq/core/offer/OpenOfferManager.java     |  9 ++--
 .../java/bisq/desktop/common/UITimer.java     | 47 +++++++++++++------
 2 files changed, 37 insertions(+), 19 deletions(-)

diff --git a/core/src/main/java/bisq/core/offer/OpenOfferManager.java b/core/src/main/java/bisq/core/offer/OpenOfferManager.java
index df5a04e774..4c58872072 100644
--- a/core/src/main/java/bisq/core/offer/OpenOfferManager.java
+++ b/core/src/main/java/bisq/core/offer/OpenOfferManager.java
@@ -159,9 +159,8 @@ public class OpenOfferManager implements PeerManager.Listener, DecryptedDirectMe
         openOfferTradableListStorage = storage;
 
         // In case the app did get killed the shutDown from the modules is not called, so we use a shutdown hook
-        Runtime.getRuntime().addShutdownHook(new Thread(() -> {
-            UserThread.execute(OpenOfferManager.this::shutDown);
-        }, "OpenOfferManager.ShutDownHook"));
+        Runtime.getRuntime().addShutdownHook(new Thread(() ->
+                UserThread.execute(OpenOfferManager.this::shutDown), "OpenOfferManager.ShutDownHook"));
     }
 
     @Override
@@ -220,7 +219,9 @@ public class OpenOfferManager implements PeerManager.Listener, DecryptedDirectMe
         int size = openOffers != null ? openOffers.size() : 0;
         log.info("Remove open offers at shutDown. Number of open offers: {}", size);
         if (offerBookService.isBootstrapped() && size > 0) {
-            openOffers.forEach(openOffer -> offerBookService.removeOfferAtShutDown(openOffer.getOffer().getOfferPayload()));
+            UserThread.execute(() -> openOffers.forEach(
+                    openOffer -> offerBookService.removeOfferAtShutDown(openOffer.getOffer().getOfferPayload())
+            ));
             if (completeHandler != null)
                 UserThread.runAfter(completeHandler, size * 200 + 500, TimeUnit.MILLISECONDS);
         } else {
diff --git a/desktop/src/main/java/bisq/desktop/common/UITimer.java b/desktop/src/main/java/bisq/desktop/common/UITimer.java
index 0d23f93ad8..e8e92901e9 100644
--- a/desktop/src/main/java/bisq/desktop/common/UITimer.java
+++ b/desktop/src/main/java/bisq/desktop/common/UITimer.java
@@ -18,8 +18,11 @@
 package bisq.desktop.common;
 
 import bisq.common.Timer;
+import bisq.common.UserThread;
 import bisq.common.reactfx.FxTimer;
 
+import javafx.application.Platform;
+
 import java.time.Duration;
 
 import org.slf4j.Logger;
@@ -34,31 +37,45 @@ public class UITimer implements Timer {
 
     @Override
     public Timer runLater(Duration delay, Runnable runnable) {
-        if (timer == null) {
-            timer = FxTimer.create(delay, runnable);
-            timer.restart();
-        } else {
-            log.warn("runLater called on an already running timer.");
-        }
+        executeDirectlyIfPossible(() -> {
+            if (timer == null) {
+                timer = FxTimer.create(delay, runnable);
+                timer.restart();
+            } else {
+                log.warn("runLater called on an already running timer.");
+            }
+        });
         return this;
     }
 
     @Override
     public Timer runPeriodically(Duration interval, Runnable runnable) {
-        if (timer == null) {
-            timer = FxTimer.createPeriodic(interval, runnable);
-            timer.restart();
-        } else {
-            log.warn("runPeriodically called on an already running timer.");
-        }
+        executeDirectlyIfPossible(() -> {
+            if (timer == null) {
+                timer = FxTimer.createPeriodic(interval, runnable);
+                timer.restart();
+            } else {
+                log.warn("runPeriodically called on an already running timer.");
+            }
+        });
         return this;
     }
 
     @Override
     public void stop() {
-        if (timer != null) {
-            timer.stop();
-            timer = null;
+        executeDirectlyIfPossible(() -> {
+            if (timer != null) {
+                timer.stop();
+                timer = null;
+            }
+        });
+    }
+
+    private void executeDirectlyIfPossible(Runnable runnable) {
+        if (Platform.isFxApplicationThread()) {
+            runnable.run();
+        } else {
+            UserThread.execute(runnable);
         }
     }
 }