From 8e8354b1ad528d57f6b3d80e44a14510325a2683 Mon Sep 17 00:00:00 2001 From: chimp1984 Date: Wed, 12 Aug 2020 20:46:17 -0500 Subject: [PATCH 1/2] Optimize application shutdown There have been several long delays as well a wrong order of the shutdown process (wallet got shutdown after network shutdown. Shutdown is now pretty fast, but depends on open offers and connections. --- .../java/bisq/core/app/BisqExecutable.java | 77 +++++++++++-------- .../bisq/core/btc/setup/WalletConfig.java | 38 ++++++--- .../bisq/core/btc/setup/WalletsSetup.java | 12 +-- .../java/bisq/network/p2p/P2PService.java | 12 ++- .../bisq/network/p2p/network/NetworkNode.java | 11 +++ 5 files changed, 97 insertions(+), 53 deletions(-) diff --git a/core/src/main/java/bisq/core/app/BisqExecutable.java b/core/src/main/java/bisq/core/app/BisqExecutable.java index 73f85dacc4f..e52aa07edbb 100644 --- a/core/src/main/java/bisq/core/app/BisqExecutable.java +++ b/core/src/main/java/bisq/core/app/BisqExecutable.java @@ -67,7 +67,7 @@ public abstract class BisqExecutable implements GracefulShutDownHandler, BisqSet protected Injector injector; protected AppModule module; protected Config config; - private boolean isShutdown = false; + private boolean isShutdownInProgress; public BisqExecutable(String fullName, String scriptName, String appName, String version) { this.fullName = fullName; @@ -204,47 +204,56 @@ protected void startAppSetup() { // This might need to be overwritten in case the application is not using all modules @Override public void gracefulShutDown(ResultHandler resultHandler) { - if (isShutdown) // prevent double cleanup + log.info("Start graceful shutDown"); + if (isShutdownInProgress) { return; + } + + isShutdownInProgress = true; + + if (injector == null) { + log.warn("Shut down called before injector was created"); + resultHandler.handleResult(); + System.exit(0); + } - isShutdown = true; try { - if (injector != null) { - injector.getInstance(ArbitratorManager.class).shutDown(); - injector.getInstance(TradeManager.class).shutDown(); - injector.getInstance(DaoSetup.class).shutDown(); - injector.getInstance(OpenOfferManager.class).shutDown(() -> { - log.info("OpenOfferManager shutdown completed"); + injector.getInstance(ArbitratorManager.class).shutDown(); + injector.getInstance(TradeManager.class).shutDown(); + injector.getInstance(DaoSetup.class).shutDown(); + injector.getInstance(AvoidStandbyModeService.class).shutDown(); + injector.getInstance(OpenOfferManager.class).shutDown(() -> { + log.info("OpenOfferManager shutdown completed"); + + injector.getInstance(BtcWalletService.class).shutDown(); + injector.getInstance(BsqWalletService.class).shutDown(); + + // We need to shutdown BitcoinJ before the P2PService as it uses Tor. + WalletsSetup walletsSetup = injector.getInstance(WalletsSetup.class); + walletsSetup.shutDownComplete.addListener((ov, o, n) -> { + log.info("WalletsSetup shutdown completed"); + injector.getInstance(P2PService.class).shutDown(() -> { log.info("P2PService shutdown completed"); - injector.getInstance(WalletsSetup.class).shutDownComplete.addListener((ov, o, n) -> { - log.info("WalletsSetup shutdown completed"); - module.close(injector); - resultHandler.handleResult(); - log.info("Graceful shutdown completed. Exiting now."); - System.exit(0); - }); - injector.getInstance(WalletsSetup.class).shutDown(); - injector.getInstance(BtcWalletService.class).shutDown(); - injector.getInstance(BsqWalletService.class).shutDown(); + + module.close(injector); + resultHandler.handleResult(); + log.info("Graceful shutdown completed. Exiting now."); + System.exit(0); }); }); - injector.getInstance(AvoidStandbyModeService.class).shutDown(); - // we wait max 20 sec. - UserThread.runAfter(() -> { - log.warn("Timeout triggered resultHandler"); - resultHandler.handleResult(); - System.exit(0); - }, 20); - } else { - log.warn("injector == null triggered resultHandler"); - UserThread.runAfter(() -> { - resultHandler.handleResult(); - System.exit(0); - }, 1); - } + walletsSetup.shutDown(); + + }); + + // Wait max 20 sec. + UserThread.runAfter(() -> { + log.warn("Timeout triggered resultHandler"); + resultHandler.handleResult(); + System.exit(0); + }, 20); } catch (Throwable t) { - log.error("App shutdown failed with exception"); + log.error("App shutdown failed with exception {}", t.toString()); t.printStackTrace(); System.exit(1); } diff --git a/core/src/main/java/bisq/core/btc/setup/WalletConfig.java b/core/src/main/java/bisq/core/btc/setup/WalletConfig.java index 5c105f9ed6e..31d68cd8caf 100644 --- a/core/src/main/java/bisq/core/btc/setup/WalletConfig.java +++ b/core/src/main/java/bisq/core/btc/setup/WalletConfig.java @@ -492,8 +492,11 @@ void setPeerNodesForLocalHost() { } } - private Wallet createOrLoadWallet(File walletFile, boolean shouldReplayWallet, - BisqKeyChainGroup keyChainGroup, boolean isBsqWallet, DeterministicSeed restoreFromSeed) + private Wallet createOrLoadWallet(File walletFile, + boolean shouldReplayWallet, + BisqKeyChainGroup keyChainGroup, + boolean isBsqWallet, + DeterministicSeed restoreFromSeed) throws Exception { if (restoreFromSeed != null) @@ -530,7 +533,9 @@ private void maybeMoveOldWalletOutOfTheWay(File vWalletFile) { } } - private Wallet loadWallet(File walletFile, boolean shouldReplayWallet, boolean useBitcoinDeterministicKeyChain) throws Exception { + private Wallet loadWallet(File walletFile, + boolean shouldReplayWallet, + boolean useBitcoinDeterministicKeyChain) throws Exception { Wallet wallet; try (FileInputStream walletStream = new FileInputStream(walletFile)) { List extensions = provideWalletExtensions(); @@ -570,21 +575,32 @@ protected void shutDown() throws Exception { // Runs in a separate thread. try { Context.propagate(context); - vPeerGroup.stop(); + vBtcWallet.saveToFile(vBtcWalletFile); - if (vBsqWallet != null && vBsqWalletFile != null) - //noinspection ConstantConditions,ConstantConditions + vBtcWallet = null; + log.info("BtcWallet saved to file"); + + if (vBsqWallet != null && vBsqWalletFile != null) { vBsqWallet.saveToFile(vBsqWalletFile); - vStore.close(); + vBsqWallet = null; + log.info("BsqWallet saved to file"); + } - vPeerGroup = null; - vBtcWallet = null; - vBsqWallet = null; + vStore.close(); vStore = null; + log.info("SPV file closed"); + vChain = null; + + // vPeerGroup.stop has no timeout and can take very long (10 sec. in my test). So we call it at the end. + // We might get likely interrupted by the parent call timeout. + vPeerGroup.stop(); + vPeerGroup = null; + log.info("PeerGroup stopped"); } catch (BlockStoreException e) { throw new IOException(e); - } catch (Throwable ignore) { + } catch (Throwable t) { + log.error(t.toString(), t); } } diff --git a/core/src/main/java/bisq/core/btc/setup/WalletsSetup.java b/core/src/main/java/bisq/core/btc/setup/WalletsSetup.java index 722799f451a..a2a94dcab9f 100644 --- a/core/src/main/java/bisq/core/btc/setup/WalletsSetup.java +++ b/core/src/main/java/bisq/core/btc/setup/WalletsSetup.java @@ -18,7 +18,6 @@ package bisq.core.btc.setup; import bisq.core.btc.exceptions.InvalidHostException; -import bisq.core.btc.nodes.LocalBitcoinNode; import bisq.core.btc.exceptions.RejectedTxException; import bisq.core.btc.model.AddressEntry; import bisq.core.btc.model.AddressEntryList; @@ -27,6 +26,7 @@ import bisq.core.btc.nodes.BtcNodes.BtcNode; import bisq.core.btc.nodes.BtcNodesRepository; import bisq.core.btc.nodes.BtcNodesSetupPreferences; +import bisq.core.btc.nodes.LocalBitcoinNode; import bisq.core.user.Preferences; import bisq.network.Socks5MultiDiscovery; @@ -314,14 +314,16 @@ public void failed(@NotNull Service.State from, @NotNull Throwable failure) { public void shutDown() { if (walletConfig != null) { try { + log.info("walletConfig shutDown started"); walletConfig.stopAsync(); - walletConfig.awaitTerminated(5, TimeUnit.SECONDS); + walletConfig.awaitTerminated(1, TimeUnit.SECONDS); + log.info("walletConfig shutDown completed"); } catch (Throwable ignore) { + log.info("walletConfig shutDown interrupted by timeout"); } - shutDownComplete.set(true); - } else { - shutDownComplete.set(true); } + + shutDownComplete.set(true); } public void reSyncSPVChain() throws IOException { diff --git a/p2p/src/main/java/bisq/network/p2p/P2PService.java b/p2p/src/main/java/bisq/network/p2p/P2PService.java index 5caab46ec40..d4dc6910908 100644 --- a/p2p/src/main/java/bisq/network/p2p/P2PService.java +++ b/p2p/src/main/java/bisq/network/p2p/P2PService.java @@ -123,6 +123,7 @@ public class P2PService implements SetupListener, MessageListener, ConnectionLis private final IntegerProperty numConnectedPeers = new SimpleIntegerProperty(0); private volatile boolean shutDownInProgress; + @Getter private boolean shutDownComplete; private final Subscription networkReadySubscription; private boolean isBootstrapped; @@ -446,7 +447,8 @@ public void onAdded(Collection protectedStorageEntries) { } @Override - public void onRemoved(Collection protectedStorageEntries) { } + public void onRemoved(Collection protectedStorageEntries) { + } /////////////////////////////////////////////////////////////////////////////////////////// // DirectMessages @@ -463,7 +465,9 @@ public void sendEncryptedDirectMessage(NodeAddress peerNodeAddress, PubKeyRing p } } - private void doSendEncryptedDirectMessage(@NotNull NodeAddress peersNodeAddress, PubKeyRing pubKeyRing, NetworkEnvelope message, + private void doSendEncryptedDirectMessage(@NotNull NodeAddress peersNodeAddress, + PubKeyRing pubKeyRing, + NetworkEnvelope message, SendDirectMessageListener sendDirectMessageListener) { log.debug("Send encrypted direct message {} to peer {}", message.getClass().getSimpleName(), peersNodeAddress); @@ -691,7 +695,9 @@ public void onBroadcastedToFirstPeer(BroadcastMessage message) { } @Override - public void onBroadcastCompleted(BroadcastMessage message, int numOfCompletedBroadcasts, int numOfFailedBroadcasts) { + public void onBroadcastCompleted(BroadcastMessage message, + int numOfCompletedBroadcasts, + int numOfFailedBroadcasts) { log.info("Broadcast completed: Sent to {} peers (failed: {}). Message = {}", numOfCompletedBroadcasts, numOfFailedBroadcasts, Utilities.toTruncatedString(message)); if (numOfCompletedBroadcasts == 0) diff --git a/p2p/src/main/java/bisq/network/p2p/network/NetworkNode.java b/p2p/src/main/java/bisq/network/p2p/network/NetworkNode.java index 8e8e48b1686..07e8d798b81 100644 --- a/p2p/src/main/java/bisq/network/p2p/network/NetworkNode.java +++ b/p2p/src/main/java/bisq/network/p2p/network/NetworkNode.java @@ -334,7 +334,17 @@ public void shutDown(Runnable shutDownCompleteHandler) { Set allConnections = getAllConnections(); int numConnections = allConnections.size(); + + if (numConnections == 0) { + log.info("Shutdown immediately because no connections are open."); + if (shutDownCompleteHandler != null) { + shutDownCompleteHandler.run(); + } + return; + } + log.info("Shutdown {} connections", numConnections); + AtomicInteger shutdownCompleted = new AtomicInteger(); Timer timeoutHandler = UserThread.runAfter(() -> { if (shutDownCompleteHandler != null) { @@ -342,6 +352,7 @@ public void shutDown(Runnable shutDownCompleteHandler) { shutDownCompleteHandler.run(); } }, 3); + allConnections.forEach(c -> c.shutDown(CloseConnectionReason.APP_SHUT_DOWN, () -> { shutdownCompleted.getAndIncrement(); From 8e3b4d912facc5bb5650911b9b50cf79bad3adc5 Mon Sep 17 00:00:00 2001 From: chimp1984 Date: Wed, 12 Aug 2020 21:23:08 -0500 Subject: [PATCH 2/2] Bow to Codacy robot I do not agree that not allowing Throwable in a catch makes the code better. Unknown exceptions can be easier found if there is an error log at the code where it occurred. I would prefer if there is some flexibility like it is the case with the IDEA code analysis, where one can edit and customize the suggestions. Ignore annotations would help. --- core/src/main/java/bisq/core/btc/setup/WalletConfig.java | 2 -- p2p/src/main/java/bisq/network/p2p/P2PService.java | 1 + 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/core/src/main/java/bisq/core/btc/setup/WalletConfig.java b/core/src/main/java/bisq/core/btc/setup/WalletConfig.java index 31d68cd8caf..3136652542e 100644 --- a/core/src/main/java/bisq/core/btc/setup/WalletConfig.java +++ b/core/src/main/java/bisq/core/btc/setup/WalletConfig.java @@ -599,8 +599,6 @@ protected void shutDown() throws Exception { log.info("PeerGroup stopped"); } catch (BlockStoreException e) { throw new IOException(e); - } catch (Throwable t) { - log.error(t.toString(), t); } } diff --git a/p2p/src/main/java/bisq/network/p2p/P2PService.java b/p2p/src/main/java/bisq/network/p2p/P2PService.java index d4dc6910908..252dcd7d694 100644 --- a/p2p/src/main/java/bisq/network/p2p/P2PService.java +++ b/p2p/src/main/java/bisq/network/p2p/P2PService.java @@ -448,6 +448,7 @@ public void onAdded(Collection protectedStorageEntries) { @Override public void onRemoved(Collection protectedStorageEntries) { + // not handled } ///////////////////////////////////////////////////////////////////////////////////////////