Added graceful shutdown hook #4047
Conversation
|
I was expecting to see the "Bisq is shutting down" modal popup, same as cmd + Q to quit, but it did not appear. Is it really doing the graceful shutdown same as cmd + Q ? Also, can you handle SIGINT as well as SIGTERM ? |
There was a problem hiding this comment.
NACK - #4047 (review)
Could you please have a look at the comment by @wiz? Thanks!
|
ah yes, I had the hopes that after achieving the same for the monitor back in the days, it would also work here. 2 issues:
stand by... @wiz: even if it works someday this will never display something in the GUI. The gui does not have the necessary infrastructure. |
Graceful shutdown has only be done in case of an error or when using the GUI. A regular eg. seednode shutdown is not covered though. Now, SIGTERM and SIGINT triggers a graceful shutdown procedure.
The close connection process did fire up worker threads to actually close the connections. Yet, once all threads have been spawned, the code proceeds assuming that there are no connections left open without checking. This lead to situations where tor has been shutdown already but open connections. These connections tried to gracefully close but without tor, that only caused a wall of exceptions.
freimair
force-pushed
the
graceful_shutdown
branch
from
e1868d7
to
2e7483d
Compare
| @@ -225,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()); | |||
There was a problem hiding this comment.
Is there a specific reason why we don't want to have this in our logs anymore?
There was a problem hiding this comment.
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).
Here, the tor object is a member variable and there are cases where this member variable is not set yet. Situation arose where a sigterm/sigint shutdown is requested and due to the member variable not set tor was left running.
The actual System::exit commands have been scattered around various places in the code. Sometimes, actual system exit depended on the calling code to reach its end of execution.
Before, the graceful shutdown procedures have been executed in the user thread. However, the sync mechanics for connections/offers caused a lockup, as some little parts of the code do execute on the user thread as well.
freimair
force-pushed
the
graceful_shutdown
branch
from
2e7483d
to
50ba2cd
Compare
Now that we can gracefully restart from bisq-network#4047
Now that we can gracefully restart from bisq-network#4047
Fixes #3884, fixes #2465
Graceful shutdown has only been done in case of an error or when
using the GUI. A regular eg. seednode shutdown is not covered
though.
Now, a
kill -TERM ...or akill -INTor[Ctrl+C]triggers a graceful shutdown procedure.Tested on a Bisq app and seednode service launch by firing up and then
kill -TERM javapidkill -INT javapid[Ctrl+C]