Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added graceful shutdown hook #4047

Merged
merged 9 commits into from
Mar 31, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 24 additions & 1 deletion core/src/main/java/bisq/core/app/BisqExecutable.java
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,10 @@

import lombok.extern.slf4j.Slf4j;



import sun.misc.Signal;

@Slf4j
public abstract class BisqExecutable implements GracefulShutDownHandler, BisqSetup.BisqSetupListener {

Expand All @@ -63,6 +67,7 @@ public abstract class BisqExecutable implements GracefulShutDownHandler, BisqSet
protected Injector injector;
protected AppModule module;
protected Config config;
private boolean isShutdown = false;

public BisqExecutable(String fullName, String scriptName, String appName, String version) {
this.fullName = fullName;
Expand Down Expand Up @@ -100,6 +105,16 @@ protected void doExecute() {
CoreSetup.setup(config);
addCapabilities();

Signal.handle(new Signal("INT"), signal -> {
gracefulShutDown(() -> {
});
});

Signal.handle(new Signal("TERM"), signal -> {
gracefulShutDown(() -> {
});
});

// If application is JavaFX application we need to wait until it is initialized
launchApplication();
}
Expand Down Expand Up @@ -189,6 +204,10 @@ 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
return;

isShutdown = true;
try {
if (injector != null) {
injector.getInstance(ArbitratorManager.class).shutDown();
Expand All @@ -214,10 +233,14 @@ public void gracefulShutDown(ResultHandler resultHandler) {
UserThread.runAfter(() -> {
log.warn("Timeout triggered resultHandler");
resultHandler.handleResult();
System.exit(0);
}, 20);
} else {
log.warn("injector == null triggered resultHandler");
UserThread.runAfter(resultHandler::handleResult, 1);
UserThread.runAfter(() -> {
resultHandler.handleResult();
System.exit(0);
}, 1);
}
} catch (Throwable t) {
log.error("App shutdown failed with exception");
Expand Down
13 changes: 10 additions & 3 deletions core/src/main/java/bisq/core/app/misc/ExecutableForAppWithP2p.java
Original file line number Diff line number Diff line change
Expand Up @@ -80,17 +80,24 @@ public void gracefulShutDown(ResultHandler resultHandler) {
injector.getInstance(OpenOfferManager.class).shutDown(() -> injector.getInstance(P2PService.class).shutDown(() -> {
injector.getInstance(WalletsSetup.class).shutDownComplete.addListener((ov, o, n) -> {
module.close(injector);
log.debug("Graceful shutdown completed");
resultHandler.handleResult();
log.info("Graceful shutdown completed");
System.exit(0);
});
injector.getInstance(WalletsSetup.class).shutDown();
injector.getInstance(BtcWalletService.class).shutDown();
injector.getInstance(BsqWalletService.class).shutDown();
}));
// we wait max 5 sec.
UserThread.runAfter(resultHandler::handleResult, 5);
UserThread.runAfter(() -> {
resultHandler.handleResult();
System.exit(0);
}, 5);
} else {
UserThread.runAfter(resultHandler::handleResult, 1);
UserThread.runAfter(() -> {
resultHandler.handleResult();
System.exit(0);
}, 1);
}
} catch (Throwable t) {
log.debug("App shutdown failed with exception");
Expand Down
4 changes: 2 additions & 2 deletions desktop/src/main/java/bisq/desktop/app/BisqApp.java
Original file line number Diff line number Diff line change
Expand Up @@ -151,11 +151,11 @@ public void stop() {
.hideCloseButton()
.useAnimation(false)
.show();
UserThread.runAfter(() -> {
new Thread(() -> {
gracefulShutDownHandler.gracefulShutDown(() -> {
log.debug("App shutdown complete");
});
}, 200, TimeUnit.MILLISECONDS);
}).start();
shutDownRequested = true;
}
}
Expand Down
6 changes: 5 additions & 1 deletion p2p/src/main/java/bisq/network/p2p/network/Connection.java
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,7 @@ public void sendMessage(NetworkEnvelope networkEnvelope) {
if (!stopped) {
synchronized (lock) {
BundleOfEnvelopes current = queueOfBundles.poll();
if (current != null) {
if (current != null && !stopped) {
if (current.getEnvelopes().size() == 1) {
protoOutputStream.writeEnvelope(current.getEnvelopes().get(0));
} else {
Expand Down Expand Up @@ -627,6 +627,10 @@ public boolean reportInvalidRequest(RuleViolation ruleViolation) {
private void handleException(Throwable e) {
CloseConnectionReason closeConnectionReason;

// silent fail if we are shutdown
if (stopped)
return;

if (e instanceof SocketException) {
if (socket.isClosed())
closeConnectionReason = CloseConnectionReason.SOCKET_CLOSED;
Expand Down
24 changes: 22 additions & 2 deletions p2p/src/main/java/bisq/network/p2p/network/NetworkNode.java
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@
import javafx.beans.property.ReadOnlyObjectProperty;
import javafx.beans.property.SimpleObjectProperty;

import java.time.Duration;
import java.time.LocalTime;

import java.net.ConnectException;
import java.net.ServerSocket;
import java.net.Socket;
Expand Down Expand Up @@ -222,7 +225,7 @@ public void onSuccess(Connection connection) {
}

public void onFailure(@NotNull Throwable throwable) {
log.info("onFailure at sendMessage: peersNodeAddress={}\n\tmessage={}\n\tthrowable={}", peersNodeAddress, networkEnvelope.getClass().getSimpleName(), throwable.toString());
log.debug("onFailure at sendMessage: peersNodeAddress={}\n\tmessage={}\n\tthrowable={}", peersNodeAddress, networkEnvelope.getClass().getSimpleName(), throwable.toString());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a specific reason why we don't want to have this in our logs anymore?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this logline is only triggered in a very limited and rare set of situations. One of them being after tor has been shutdown already (but not if tor is gone without a trigger from Bisq). Plus, this log line does not help support. Iff the info is necessary, --logLevel=DEBUG is there.

Generally, log outputs are scattered throughout the p2p package. I am currently working on a battle plan to refactor the message handling in order to get more reliability and control over what happens (we do have a lost message every now and then, and I found hints on why that is).

UserThread.execute(() -> resultFuture.setException(throwable));
}
});
Expand Down Expand Up @@ -330,7 +333,24 @@ public void shutDown(Runnable shutDownCompleteHandler) {
server = null;
}

getAllConnections().stream().forEach(c -> c.shutDown(CloseConnectionReason.APP_SHUT_DOWN));
getAllConnections().parallelStream().forEach(c -> c.shutDown(CloseConnectionReason.APP_SHUT_DOWN));

// wait for connections to actually close, c.shutDown may create threads to actually close connections...
LocalTime timeout = LocalTime.now().plus(Duration.ofSeconds(15));
while (!getAllConnections().isEmpty()) {
// check timeout
if (timeout.isBefore(LocalTime.now())) {
log.error("Could not close all connections within timeout (" + getAllConnections().size() + " connections remaining). Moving on.");
break;
}
try {
// reduce system load
Thread.sleep(10);
} catch (InterruptedException e) {
// ignore
}
}

log.debug("NetworkNode shutdown complete");
}
if (shutDownCompleteHandler != null) shutDownCompleteHandler.run();
Expand Down
12 changes: 11 additions & 1 deletion p2p/src/main/java/bisq/network/p2p/network/TorNetworkNode.java
Original file line number Diff line number Diff line change
Expand Up @@ -148,8 +148,10 @@ public Socks5Proxy getSocksProxy() {
}

public void shutDown(@Nullable Runnable shutDownCompleteHandler) {
BooleanProperty torNetworkNodeShutDown = torNetworkNodeShutDown();
// this one is executed synchronously
BooleanProperty networkNodeShutDown = networkNodeShutDown();
// this one is committed as a thread to the executor
BooleanProperty torNetworkNodeShutDown = torNetworkNodeShutDown();
BooleanProperty shutDownTimerTriggered = shutDownTimerTriggered();

// Need to store allShutDown to not get garbage collected
Expand Down Expand Up @@ -185,6 +187,14 @@ private BooleanProperty torNetworkNodeShutDown() {
long ts = System.currentTimeMillis();
log.debug("Shutdown torNetworkNode");
try {
/**
* make sure we get tor.
* - there have been situations where <code>tor</code> isn't set yet, which would leave tor running
* - downside is that if tor is not already started, we start it here just to shut it down. However,
* that can only be the case if Bisq gets shutdown even before it reaches step 2/4 at startup.
* The risk seems worth it compared to the risk of not shutting down tor.
*/
tor = Tor.getDefault();
if (tor != null)
tor.shutdown();
log.debug("Shutdown torNetworkNode done after " + (System.currentTimeMillis() - ts) + " ms.");
Expand Down