Handle CTRL+C gracefully #10529
Handle CTRL+C gracefully #10529
Conversation
There was a problem hiding this comment.
tACK 2c3e8d5
CTRL C on Debian works better now.
For Quit Wasabi not working, idk as idk how to repro.
…Rework-app-termination # Conflicts: # WalletWasabi.Daemon/WasabiAppBuilder.cs
| MainViewModel.Instance.ClearStacks(); | ||
| MainViewModel.Instance.StatusIcon.Dispose(); | ||
|
|
||
| AppLifetimeHelper.Shutdown(withShutdownPrevention: false, restart: false); |
There was a problem hiding this comment.
Can you explain these 2 changes, please? I do not understand them.
There was a problem hiding this comment.
Dispatcher.UIThread.Post was used to run the L87-L92 on the UI thread. UI frameworks work with single-threaded approach where all UI changes happen on the UI thread (you are obviously free to use threads as you like but you must make sure that the results you want to display are published using the UI thread)..
Without Dispatcher.UIThread.Post, I don't think the TerminateApplication() method was correct as there was no guarrantee that it is really the UI thread accessing MainViewModel.
So that's the first change.
The second change was that I added AppLifetimeHelper.Shutdown(withShutdownPrevention: false, restart: false); to force Avalonia to stop. Essentially, it makes sure that we move from:
to
and that in turn means that we gracefully continue by executing https://github.com/kiminuo/WalletWasabi/blob/b1629638782ef445c7333a719636b57cc632acbc/WalletWasabi.Daemon/WasabiAppBuilder.cs#L61-L66
|
It doesn't finish nicely when stopped using the rpc Update: You can test it with wcli.sh as follow: Note: make sure to have the json rpc enabled. You can use the |
There was a problem hiding this comment.
tACK b162963
I do get this error with Ctrl+C in terminal
^C2023-04-21 18:14:33.523 [25] WARNING TerminateService.Console_CancelKeyPress (85) Process termination was requested using 'ControlC' keyboard shortcut.
2023-04-21 18:14:33.641 [23] ERROR TorControlClient.ReaderLoopAsync (431) Incomplete Tor control reply was received. Tor probably terminated abruptly. Exception: WalletWasabi.Tor.Control.Exceptions.TorControlReplyParseException: No reply line was received.
---> System.IO.InvalidDataException: No more data.
at WalletWasabi.Tor.Control.PipeReaderLineReaderExtension.ReadLineAsync(PipeReader reader, CancellationToken cancellationToken) in WalletWasabi/Tor/Control/PipeReaderLineReaderExtension.cs:line 95
at WalletWasabi.Tor.Control.TorControlReplyReader.ReadReplyAsync(PipeReader reader, CancellationToken cancellationToken) in WalletWasabi/Tor/Control/TorControlReplyReader.cs:line 31
--- End of inner exception stack trace ---
at WalletWasabi.Tor.Control.TorControlReplyReader.ReadReplyAsync(PipeReader reader, CancellationToken cancellationToken) in WalletWasabi/Tor/Control/TorControlReplyReader.cs:line 39
at WalletWasabi.Tor.Control.TorControlClient.ReaderLoopAsync() in WalletWasabi/Tor/Control/TorControlClient.cs:line 389
When stopping with the RPC, I get
2023-04-21 18:15:30.333 [11] INFO CoinJoinManager.WaitAndHandleResultOfTasksAsync (557) Task 'walletsMonitoringTask' finished successfully by cancellation.
2023-04-21 18:15:30.333 [6] INFO CoinJoinManager.WaitAndHandleResultOfTasksAsync (557) Task 'commandsHandlingTask' finished successfully by cancellation.
2023-04-21 18:15:30.333 [8] WARNING Global.DisposeAsync (340) Process is exiting.
2023-04-21 18:15:30.333 [6] INFO CoinJoinManager.WaitAndHandleResultOfTasksAsync (557) Task 'monitorCoinJoinTask' finished successfully by cancellation.
2023-04-21 18:15:30.335 [6] INFO CoinJoinManager.WaitAndHandleResultOfTasksAsync (553) Task 'monitorAndHandleCoinjoinsTask' finished successfully.
2023-04-21 18:15:51.448 [20] INFO TerminateService.Terminate (152) Wasabi stopped gracefully (a2c222a3-02d8-4c9e-9533-d428ab319495).
2023-04-21 18:15:51.451 [20] ERROR JsonRpcServer.ExecuteAsync (90) System.ObjectDisposedException: Cannot access a disposed object.
Object name: 'System.Net.HttpListenerResponse'.
at System.Net.HttpListenerResponse.set_ContentType(String value)
at WalletWasabi.Rpc.JsonRpcServer.ExecuteAsync(CancellationToken stoppingToken) in WalletWasabi/Rpc/JsonRpcServer.cs:line 66
2023-04-21 18:15:51.487 [1] INFO WasabiAppBuilder.BeforeStopping (87) Wasabi GUI stopped gracefully (a2c222a3-02d8-4c9e-9533-d428ab319495).
|
@lontivero Isn't it in fact a
Then, we'll get an tl;dr: Footnotes
|
|
Regarding #10529 (review), I think we should make the error message better by detecting that shutdown is happening. I don't think it's new with this PR, so I would like to tackle it in a follow-up PR. |
It's not new you're right: #10499 (review)
ACK |
It can be. My point is that it doesn't happen in master. |
I am reproducing it every time on See logs:
|
|
Thanks @turbolay, i assumed it didn't happen because i was playing with the rpc server all the week but yes, it is true, it was already broken. Ok. ACK. |
There was a problem hiding this comment.
I tested again and apparently, I forgot to test the Daemon? Because it seems that it's not working, at least on macOs.
I changed the code that way to make it work:
- Make
WasabiAppBuilder.Terminatenull instead of empty (so it will be null if it's the Daemon) and call basicTerminatewithinConsole_CancelKeyPressif it's null - Still set
e.Cancel = true; because otherwise thedotnetprocess still hangs
diff --git a/WalletWasabi.Daemon/WasabiAppBuilder.cs b/WalletWasabi.Daemon/WasabiAppBuilder.cs
index 640fbf181..c263db455 100644
--- a/WalletWasabi.Daemon/WasabiAppBuilder.cs
+++ b/WalletWasabi.Daemon/WasabiAppBuilder.cs
@@ -146,7 +146,7 @@ public record WasabiAppBuilder(string AppName, string[] Arguments)
internal bool MustCheckSingleInstance { get; init; }
internal EventHandler<Exception>? UnhandledExceptionEventHandler { get; init; }
internal EventHandler<AggregateException>? UnobservedTaskExceptionsEventHandler { get; init; }
- internal Action Terminate { get; init; } = () => { };
+ internal Action? Terminate { get; init; }
public WasabiAppBuilder EnsureSingleInstance(bool ensure = true) =>
this with { MustCheckSingleInstance = ensure };
diff --git a/WalletWasabi/Services/Terminate/TerminateService.cs b/WalletWasabi/Services/Terminate/TerminateService.cs
index dfbb07fe8..1da7d6942 100644
--- a/WalletWasabi/Services/Terminate/TerminateService.cs
+++ b/WalletWasabi/Services/Terminate/TerminateService.cs
@@ -15,10 +15,10 @@ public class TerminateService
private const long TerminateStatusInProgress = 1;
private const long TerminateStatusFinished = 2;
private readonly Func<Task> _terminateApplicationAsync;
- private readonly Action _terminateApplication;
+ private readonly Action? _terminateApplication;
private long _terminateStatus;
- public TerminateService(Func<Task> terminateApplicationAsync, Action terminateApplication)
+ public TerminateService(Func<Task> terminateApplicationAsync, Action? terminateApplication)
{
_terminateApplicationAsync = terminateApplicationAsync;
_terminateApplication = terminateApplication;
@@ -84,14 +84,21 @@ public class TerminateService
{
Logger.LogWarning($"Process termination was requested using '{e.SpecialKey}' keyboard shortcut.");
- // Do not kill the process ...
- e.Cancel = true;
-
- // ... instead signal back that the app should terminate.
+ // Signal back that the app should terminate ...
if (TerminationRequested.TrySetResult())
{
// Run this callback just once.
- _terminateApplication();
+ if (_terminateApplication is not null)
+ {
+ // ... and do not kill the process
+ e.Cancel = true;
+ _terminateApplication();
+ }
+ else
+ {
+ // ... except if it's the daemon
+ Terminate();
+ }
}
}
@@ -119,7 +126,7 @@ public class TerminateService
// We want to call the callback once. Not multiple times.
if (!TerminationRequested.Task.IsCompleted)
{
- _terminateApplication();
+ _terminateApplication?.Invoke();
}
// Async termination has to be started on another thread otherwise there is a possibility of deadlock.
|
@turbolay Thank you. Actually, it's funny. I have tested Daemon but then I merged master and I have incorrectly fixed the merge conflict. I fixed it now. FTR, it was broken for all platforms on Daemon, not only macOS. Regarding the suggested code, I had a different approach in mind. |
|
The original issue was fixed - gj! However I noticed, the daemon is not quitting, starting from the terminal with dotnet run. It stuck here
BTW it behaves the same for the desktop. |
|
With the most recent master commit a50dec2, I tried to run daemon using:
and it works for me. I can stop the application with CTRL+C. Were you coinjoining? |
|
I am using powershell on windows.
|
|
I have failed to reproduce it so far. I tried PowerShell as well (Windows Powershell & PowerShell 7+). @yahiheb @adamPetho Can you reproduce #10529 (comment) as well please? edit: I have tried on Ubuntu 23.04 and I can use the quit icon just fine. So there must be a factor that affects this. |
I tried a couple of times, but couldn't reproduce it with PowerShell. I can reproduce it if I run Wasabi from Visual Studio with Debugging. |
Fixes #10448.
Possibly related to #10325
Currently, CTRL+C is imho handle in a way that it's sort of unclear what really happens (what the code flow is) and how many times we really call
TerminateService.Terminateand obviously it leads to weird state because we hit #10325 (it happens every time for me).The idea of this PR is: Handle CTRL+C interruption by just signalling to the main application that it should terminate. So the event itself is cancelled and CTRL+C does not kill process itself. Instead, we start graceful termination.
btw: This PR makes it so that we continue here https://github.com/kiminuo/WalletWasabi/blob/031f8b9efe5c962715120ad68ae076d36dd8d654/WalletWasabi.Fluent.Desktop/Program.cs#L194 when user presses CTRL+C. It seems quite nice because the code flow makes actually some sense.
Reproducing the issue
Just start the Fluent.Desktop or the Daemon project from MSVS and stop the application using CTRL+C. It hangs for me every time.
Testing
Tested on Windows, macOS.