UI & graceful shutdown support

[kiminuo]

I implemented a several PRs to handle CTRL+C gracefully. However, there are still issues, like #10814 and #10325.

So to move forward, I would like to discuss:

1. how we can make sure that UI-related tasks stop before we start disposing services in Global.cs. Something like adding StopUiTasks() here so that one knows that UI cannot touch services that are being disposed.
2. how to make sure that these 2 Task.Delay calls so that it accepts a cancellation token:
   • https://github.com/zkSNACKs/WalletWasabi/blob/1764d664303b3d49bc0dba27f10fd7a773ced6aa/WalletWasabi.Fluent/ViewModels/Wallets/LoadingViewModel.cs#L103-L106
   • https://github.com/zkSNACKs/WalletWasabi/blob/1764d664303b3d49bc0dba27f10fd7a773ced6aa/WalletWasabi.Fluent/ViewModels/Wallets/LoadingViewModel.cs#L113C36-L116
   • One idea was to add a cancellation token to UiContext (with the meaning "application is stopping"). Not sure if there is a better way.

I had a chat with @wieslawsoltes and he said that it is a good time to solve these issues because the UI decoupling PRs are being worked on (tracked under #10761).

This issue was filed for discussing purposes. Please share your honest opinion.

WDYT? @SuperJMN @ichthus1604 @soosr @wieslawsoltes (anyone else to ping?)

[soosr]

IMO all the Task.Delay logic should be removed. Instead of waiting, only execute the logic when we know it makes sense. For Example when an event comes from the business side "all filters are downloaded".

it is a good time to solve these issues because the UI decoupling PRs are being worked on

IMO the fix should be done once the UI decoupling is done on this part, otherwise, it might cause conflicts.

[kiminuo]

I agree that that would be much better fix though I wonder when that UI decoupling is supposed to be finished. What time frame are we talking about?

[soosr]

I agree that that would be much better fix though I wonder when that UI decoupling is supposed to be finished. What time frame are we talking about?

PR that touches it just got merged. You can start to fix if you wish in this file:
https://github.com/zkSNACKs/WalletWasabi/blob/b0617041ed9c3c20d796fcb8c19453a2693be4de/WalletWasabi.Fluent/Models/Wallets/WalletLoadWorkflow.cs

[ichthus1604]

IMO all the Task.Delay logic should be removed. Instead of waiting, only execute the logic when we know it makes sense. For Example when an event comes from the business side "all filters are downloaded".

I think it makes most sense to start by fixing this, then moving on to app startup logic and so forth.

I'll write a PR for it.

[soosr]

@ichthus1604 What is the status of this?

[soosr]

The second part of this issue is done.

The remaining task is:

1. how we can make sure that UI-related tasks stop before we start disposing services in Global.cs. Something like adding StopUiTasks() here so that one knows that UI cannot touch services that are being disposed.

@kiminuo Can you elaborate more on this?
At first glance, it doesn't seem good to me to put any UI-related code there, as the Daemon is totally independent of the UI.

[kiminuo]

The second part of this issue is done.

The remaining task is:

1. how we can make sure that UI-related tasks stop before we start disposing services in Global.cs. Something like adding StopUiTasks() here so that one knows that UI cannot touch services that are being disposed.

@kiminuo Can you elaborate more on this? At first glance, it doesn't seem good to me to put any UI-related code there, as the Daemon is totally independent of the UI.

At the moment, we start initializing stuff in Global.cs and then we start an Avalonia application.

Now I'm saying that right after the line https://github.com/zkSNACKs/WalletWasabi/blob/574080bd3b62855ef8d637c3be4e1071aea5491f/WalletWasabi.Fluent.Desktop/Program.cs#L206 all Avalonia UI stuff should be stopped. I'm talking mainly about that reactive stuff that reacts to, eg, synchronizer downloaded a new filter etc. If we reach that point we can safely dispose Global and all its services. The thing we want to avoid is that a UI task accesses a service defined in Global that is being disposed because then we will most likely crash in one way or another.

Maybe with the UI decoupling stuff you have some rules that makes sure that this (ie UI task accesses a service defined in Global that is being disposed) does not happen anymore but it was certainly the case in the past.

It's worth noting that Avalonia 11 seems to have better support for an application lifetime (new API methods) than Avalonia 10.

Another thing that's worth noting is that we use Avalonia package in a non-standard way (we initialize Global and then start an Avalonia app rather than starting an Avalonia app and initializing then).

[soosr]

I'm talking mainly about that reactive stuff that reacts to, eg, synchronizer downloaded a new filter etc. If we reach that point we can safely dispose Global and all its services. The thing we want to avoid is that a UI task accesses a service defined in Global that is being disposed because then we will most likely crash in one way or another.

I think this is already happening like that.
TerminateService has two termination method:

• _terminateApplicationAsync
  • This is the one that one where Global gets disposed.
    https://github.com/zkSNACKs/WalletWasabi/blob/ae9a1fe79af7910bfc7841b061ad88a17a69131e/WalletWasabi.Daemon/WasabiAppBuilder.cs#L128-L136
• _terminateApplication
  • This is the one where the UI cleanup is happening
    https://github.com/zkSNACKs/WalletWasabi/blob/ae9a1fe79af7910bfc7841b061ad88a17a69131e/WalletWasabi.Fluent.Desktop/Program.cs#L85-L96
  • The important part here is ClearStacks() because it means all the viewmodels will be removed from the NavigationStacks, and when it happens, the viewmodels will cancel the subscriptions (if there was no mistake made by the developer).

So in TerminateService, first it calls the UI terminate one (L141), then the Global one (L150).

https://github.com/zkSNACKs/WalletWasabi/blob/ae9a1fe79af7910bfc7841b061ad88a17a69131e/WalletWasabi/Services/Terminate/TerminateService.cs#L138-L156

So it shouldn't be a problem, right?

[kiminuo]

I've taken a look at it and while what you say makes sense to me, I think we are not quite there yet because:

• It's not clear to me why https://github.com/zkSNACKs/WalletWasabi/blob/ae9a1fe79af7910bfc7841b061ad88a17a69131e/WalletWasabi.Fluent.Desktop/Program.cs#L94 this is necessary if the application shuts down normally. More importantly, I have just tried to shut down a WW instance normally (by X button) and this https://github.com/zkSNACKs/WalletWasabi/blob/ae9a1fe79af7910bfc7841b061ad88a17a69131e/WalletWasabi.Fluent.Desktop/Program.cs#L89-L94 does not get called at all. So that makes me think that there is no UI cleanup as you say in the most common case of "user stops WW by clicking the X button". Can you repro?
• Also https://github.com/zkSNACKs/WalletWasabi/blob/ae9a1fe79af7910bfc7841b061ad88a17a69131e/WalletWasabi.Daemon/WasabiAppBuilder.cs#L91 is called after https://github.com/zkSNACKs/WalletWasabi/blob/ae9a1fe79af7910bfc7841b061ad88a17a69131e/WalletWasabi.Fluent.Desktop/Program.cs#L206 is done executing.
• The way how https://github.com/zkSNACKs/WalletWasabi/blob/ae9a1fe79af7910bfc7841b061ad88a17a69131e/WalletWasabi.Daemon/WasabiAppBuilder.cs#L128-L136 is called feels very obfuscated to me as well.

-> So I believe we are on the right track but the details feel weird to me and it feels it's overly complicated at the moment.

[soosr]

https://github.com/zkSNACKs/WalletWasabi/blob/ae9a1fe79af7910bfc7841b061ad88a17a69131e/WalletWasabi.Fluent.Desktop/Program.cs#L89-L94
does not get called at all. So that makes me think that there is no UI cleanup as you say in the most common case of "user stops WW by clicking the X button". Can you repro?

I can repro. Actually a breakpoint before Dispatcher.UIThread.Post(() => got hit, but a breakpoint inside of that did not.

[soosr]

https://github.com/zkSNACKs/WalletWasabi/blob/ae9a1fe79af7910bfc7841b061ad88a17a69131e/WalletWasabi.Fluent.Desktop/Program.cs#L89-L94

does not get called at all. So that makes me think that there is no UI cleanup as you say in the most common case of "user stops WW by clicking the X button". Can you repro?

I can repro. Actually a breakpoint before Dispatcher.UIThread.Post(() => got hit, but a breakpoint inside of that did not.

@kiminuo Using RxApp.MainThreadScheduler.Schedule(() => instead of Dispatcher.UIThread.Post(() => fixes it and it gets called.
@wieslawsoltes Any idea why is that?

[wieslawsoltes]

Dispatcher.UIThread

https://github.com/zkSNACKs/WalletWasabi/blob/ae9a1fe79af7910bfc7841b061ad88a17a69131e/WalletWasabi.Fluent.Desktop/Program.cs#L89-L94

does not get called at all. So that makes me think that there is no UI cleanup as you say in the most common case of "user stops WW by clicking the X button". Can you repro?

I can repro. Actually a breakpoint before Dispatcher.UIThread.Post(() => got hit, but a breakpoint inside of that did not.

@kiminuo Using RxApp.MainThreadScheduler.Schedule(() => instead of Dispatcher.UIThread.Post(() => fixes it and it gets called. @wieslawsoltes Any idea why is that?

Did not check anything yet, but my guess would be there is no more UI dispatcher running jobs during that point of time.

[kiminuo]

https://github.com/zkSNACKs/WalletWasabi/blob/ae9a1fe79af7910bfc7841b061ad88a17a69131e/WalletWasabi.Fluent.Desktop/Program.cs#L89-L94

does not get called at all. So that makes me think that there is no UI cleanup as you say in the most common case of "user stops WW by clicking the X button". Can you repro?

I can repro. Actually a breakpoint before Dispatcher.UIThread.Post(() => got hit, but a breakpoint inside of that did not.

@kiminuo Using RxApp.MainThreadScheduler.Schedule(() => instead of Dispatcher.UIThread.Post(() => fixes it and it gets called. @wieslawsoltes Any idea why is that?

Yeah, I remember that I played with that a while back and Dispatcher is stopped sooner than RxApp.MainThreadScheduler.

However, I'm not persuaded it's actually the solution we are looking for as I would expect that if WW is terminated using "X" button we would not need any dispatcher in such case (why should we need it?). That makes me think that we should identify the proper place when to

MainViewModel.Instance.ClearStacks(); 
MainViewModel.Instance.StatusIcon.Dispose(); 

Can we call these two commands simply after this line https://github.com/zkSNACKs/WalletWasabi/blob/e8d7bc0de5cce55f470f9df9fca997ae26592e8f/WalletWasabi.Fluent.Desktop/Program.cs#L206 ? Or does

MainViewModel.Instance.ClearStacks(); 
MainViewModel.Instance.StatusIcon.Dispose(); 

require Avalonia to be actually running in some way?

[soosr]

I would expect that if WW is terminated using "X" button we would not need any dispatcher in such case (why should we need it?).

I think it is actually a leftover, because previously the termination methods weren't separated. And it threw an exception due to it was executed on non UI thread.

https://github.com/zkSNACKs/WalletWasabi/blob/fe2bcd19748981875e4981f78352bf618a8012f1/WalletWasabi/Services/Terminate/TerminateService.cs#L144

Testing it now without the dispatcher, I don't see any exception.

[kiminuo]

It was modified here https://github.com/zkSNACKs/WalletWasabi/pull/10529/files#r1173029462 and I added some explanation.

I think it is actually a leftover, [..]

No. There is, for example, this line https://github.com/zkSNACKs/WalletWasabi/blob/f1b56713c893626a3f6102fcee8b35b4685318bb/WalletWasabi/Services/Terminate/TerminateService.cs#L49 and documentation clearly states:

Do not make assumptions about the thread the event is raised on. The event can be raised on a different thread than the one that called the Unload method.

And while it's true that some of my PRs regarding CTRL+C behavior has probably made that dispatch call (Dispatcher.UIThread.Post(()) almost not needed, it's still needed as of now I think.

So I think more correct approach is to move these two lines:

MainViewModel.Instance.ClearStacks();
MainViewModel.Instance.StatusIcon.Dispose();

The question is where to. This seems to work #11511 but maybe Avalonia has some sort of callback which is more fitting. WDYT?

[soosr]

The question is where to. This seems to work #11511 but maybe Avalonia has some sort of callback which is more fitting. WDYT?

I don't know about it, @wieslawsoltes?

[Pule08]

Hello @kiminuo
could you please update me, is this issue already done, or not yet?
#11511

[kiminuo]

There were multiple PRs created to fix this "meta" issue and AFAIK it works OK as of now. I'm not aware of anything else that would need to be done for this issue to be closed. There are things that we can improve (nice to have) but it's not blocking right now.

So I'm closing the issue now.