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

Stop application if CTLR+C is requested early #11036

Merged

Conversation

kiminuo
Copy link
Collaborator

@kiminuo kiminuo commented Jul 15, 2023

Contributes to #10814

This is a result of work done earlier (#10929). It's the only way I have found so far to stop the software if CTRL+C is pressed very early in the process of starting the software. Currently, on the master, CTRL+C can be ignored if it is requested "too soon".

It's still not 100 per cent correct but I'm not aware of a better solution at the moment.

@kiminuo kiminuo force-pushed the feature/2023-07-15-Stop-before-UI-started branch from b387cf1 to f0deeda Compare July 15, 2023 14:50
@kiminuo kiminuo force-pushed the feature/2023-07-15-Stop-before-UI-started branch from 85c3e81 to 46bba39 Compare July 15, 2023 19:06
@kiminuo kiminuo marked this pull request as ready for review July 17, 2023 06:26
Copy link
Collaborator

@MarnixCroes MarnixCroes left a comment

Choose a reason for hiding this comment

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

fixes the issue.
it happens rarely (like 5%) of the time that the terminal hangs (stopping software when GUI is launched)

video
Screencast.from.18-07-2023.09.42.20.webm

other than that it works better, so
ACK

@kiminuo
Copy link
Collaborator Author

kiminuo commented Jul 18, 2023

it happens rarely (like 5%) of the time that the terminal hangs (stopping software when GUI is launched)

Are you saying that this PR makes the terminal hangs in 5% cases? Or does that happen even on master?

@MarnixCroes
Copy link
Collaborator

it happens rarely (like 5%) of the time that the terminal hangs (stopping software when GUI is launched)

Are you saying that this PR makes the terminal hangs in 5% cases? Or does that happen even on master?

Correct. I cannot repro the hang on master.

@ichthus1604
Copy link
Collaborator

While testing this PR, I managed to crash Wasabi and got index corruption. I could reproduce it only once though.

@kiminuo kiminuo marked this pull request as draft July 19, 2023 07:05
@kiminuo
Copy link
Collaborator Author

kiminuo commented Jul 19, 2023

@ichthus1604

While testing this PR, I managed to crash Wasabi and got index corruption. I could reproduce it only once though.

I think you may have hit an issue the filter sync changes that were introduced recently. It's being worked on.

My understanding is: This PR attempts to stop initialization of the program which in itself is correct but not all components respond correctly. (No proof, just my gut feeling.) The question is what is better whether to hide the error or rather expose it which would force us to fix it. Personally, I like the later variant more.

However, it makes sense to me to tackle #11065 first and then we can re-test this. But it will take time.

.StartWithClassicDesktopLifetime(app.AppConfig.Arguments);
});

if (app.TerminateService.CancellationToken.IsCancellationRequested)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If user requested to stop the application "early", we don't start Avalonia actually.

private CancellationTokenSource TerminationCts { get; } = new();

/// <remarks>Assigned once so that there are no issues with <see cref="TerminationCts"/> being disposed.</remarks>
public CancellationToken CancellationToken { get; }
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is an application-wide cancellation token that denotes that user requested the application to terminate.

@kiminuo kiminuo marked this pull request as ready for review August 25, 2023 14:12
@kiminuo
Copy link
Collaborator Author

kiminuo commented Aug 25, 2023

@MarnixCroes @ichthus1604 Would you mind testing again?

Regarding #11036 (comment), filters are handled differently now so hopefully, that's fixed by now. I have not been able to reproduce that.

btw: This is still only a partial fix (90% of cases?). There is still a small time window when this will not work correctly, but to make it work every time, I think we will need Avalonia 11 where there is richer API for "shutdowns" (or some novel idea how to fix it).

Copy link
Collaborator

@MarnixCroes MarnixCroes left a comment

Choose a reason for hiding this comment

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

btw: This is still only a partial fix (90% of cases?). There is still a small time window when this will not work correctly,

sometimes it would hang if I press CTRL C after TorMonitor.MonitorEventAsync (150) Tor circuit was established.
if that's that time window you mentioned:

tACK aa024d9

@kiminuo kiminuo requested a review from molnard August 28, 2023 12:56
Copy link
Collaborator

@adamPetho adamPetho left a comment

Choose a reason for hiding this comment

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

I couldn't make the process hang, no matter how many times I tried on Win11.
Code LGTM

Copy link
Collaborator

@Szpoti Szpoti left a comment

Choose a reason for hiding this comment

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

LGTM
Tested on Win10 but couldn't repro what happened on linux.

molnard

This comment was marked as outdated.

@turbolay
Copy link
Collaborator

turbolay commented Sep 12, 2023

I didn't manage to repro behavior @molnard mentioned on Mac.
It always work great on my machine.

I know this is unrelated, but it doesn't solve the issue of quitting not working properly during Initial Transaction Processing or Wallet Synchronization as a whole.

@molnard
Copy link
Collaborator

molnard commented Sep 12, 2023

Sorry, I was in the wrong repository. My bad. Testing it again.

Copy link
Collaborator

@molnard molnard left a comment

Choose a reason for hiding this comment

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

tACK

@molnard molnard merged commit 794b972 into WalletWasabi:master Sep 12, 2023
7 checks passed
@kiminuo kiminuo deleted the feature/2023-07-15-Stop-before-UI-started branch September 12, 2023 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants