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

TryShutdown combined with unconditional e.Cancel = true stucks in the infinite loop #15533

Open
robloo opened this issue Apr 26, 2024 · 4 comments
Assignees
Labels
bug by-design The behavior reported in the issue is actually correct. enhancement

Comments

@robloo
Copy link
Contributor

robloo commented Apr 26, 2024

Describe the bug

Intercepting window close and then manually calling IClassicDesktopStyleApplicationLifetime.TryShutdown() no longer works in 11.1-beta2

To Reproduce

The main Window intercepts closing:

/// <inheritdoc/>
protected override void OnClosing(WindowClosingEventArgs e)
{
    App.Current.RunAppClosingLogic(isClosingByUser: true);

    e.Cancel = true;
    return;
}

Then, if the user confirms they wish to close without saving, it continues. Note that a lot of code is removed here and only the bare minimum kept to show the code paths taken.

public void RunAppClosingLogic(bool isClosingByUser)
{
    // User pressed 'Don't Save', go ahead and close the application
    DeleteWorkingFileAndExit();

    async void DeleteWorkingFileAndExit()
    {
        this.CloseApp();
        return;
    }

    return;
}

public void CloseApp()
{
    if (this.ApplicationLifetime is IClassicDesktopStyleApplicationLifetime desktop)
    {
        desktop.TryShutdown();
    }
    else if (this.ApplicationLifetime is ISingleViewApplicationLifetime singleView)
    {
        // Nothing is possible here
    }

    return;
}

Expected behavior

Desktop apps should be able to shutdown themselves

Avalonia version

11.1-beta2

OS

Windows

Additional context

This has worked since 11.0 previews (for over a year) and only now seems broken in 11.1-beta2.

@robloo robloo added the bug label Apr 26, 2024
@maxkatz6 maxkatz6 self-assigned this Apr 27, 2024
@maxkatz6
Copy link
Member

@robloo I thought it's a regression from #15438, but I am not sure anymore.

First of all, this specific code you provided has two issues:

  1. It's not allowed to call TryShutdown during shutdown, app will throw "Application is already shutting down" (old behavior). In your code there is no delay in DeleteWorkingFileAndExit. But I assume your real app an actual dialog.
  2. If I simulate some delay in DeleteWorkingFileAndExit, it still works as expected (kinda?). TryShutdown is called, OnClosing is executed again, cancelled again (you always set e.Cancel = true), and TryShutdown is called again after delay - trapping app in the loop.

This loop from #2 is what was changed between versions.

In 11.1-beta1 and before we had an issue that TryShutdown was closing the app even if Window.Closing was cancelled. It still was trapping in the loop of cancellation, but Dispatcher.UIThread.InvokeShutdown eventually was killing the app.

In 11.1-beta2 it was fixed, and TryShutdown now actually respects e.Cancel from Window.Closing.

@maxkatz6
Copy link
Member

I am going to remove "regression" label, as it's not an actual regression, but app relied on the previously broken behavior.

If you need to force app to be closed (ignoring Window.Closing event), you need to use Lifetime.Shutdown method (without Try). The difference is that Shutdown method actually forces app to be closed, exactly what is needed in your use case, I assume. It forces closing by not calling ShutdownRequested and Window.Closing events that can be cancelled.

But I will keep this issue opened, as:

  1. Difference between Shutdown/TryShutdown is not always clear.
  2. Ideally there should be an error or warning, when app is stack in "closing cancellation loop".

Issue title should be adjusted too.

@maxkatz6 maxkatz6 changed the title IClassicDesktopStyleApplicationLifetime.TryShutdown() Broken in 11.1-beta2 TryShutdown combined with unconditional e.Cancel = true stucks in the infinite loop Apr 27, 2024
@maxkatz6
Copy link
Member

Or we can create another issue instead, and close this one.

@robloo
Copy link
Contributor Author

robloo commented Apr 27, 2024

It's not allowed to call TryShutdown during shutdown, app will throw "Application is already shutting down" (old behavior). In your code there is no delay in DeleteWorkingFileAndExit. But I assume your real app an actual dialog.

Yes, there are dialogs in the full code. Note that I would have expected intercepting Window close to be separate than application shutdown so TryShutdown() is not actually called within TryShutdown().

In other words, when a Window is closing itself I would expect this to occur BEFORE the application shutdown process. Only once all windows are closed would Avalonia then trigger application shutdown. What this means is if you stop a window from closing TryShutdown() is never actually called because the application shutdown process never started. This makes more sense to me and I suspect is how WPF/UWP work in this area (this code was ported from UWP/WinUI where it worked there).

If I simulate some delay in DeleteWorkingFileAndExit, it still works as expected (kinda?). TryShutdown is called, OnClosing is executed again, cancelled again (you always set e.Cancel = true), and TryShutdown is called again after delay - trapping app in the loop.

Again, I am handling window close which I would expect to occur before any actual shutdown.

So I have a few high-level thoughts here: The distinction in Avalonia between Application, TopLevel and Window is almost certainly confusing/wrong in several cases.

  • For example, services are "global" and I would expect them to be available from the Application class rather than TopLevel.. another topic
  • Application closing should be fully handled in the Application class itself rather than Window
  • Cancelling window close should not trigger application shutdown
  • The top caption buttons need to route to the window and be processed there FIRST. This ensures pressing X starts window close rather than application shutdown.

Difference between Shutdown/TryShutdown is not always clear.

Yea, I wasn't aware we had two different methods for this and in my code just using Shutdown should solve the actual issue.

I would leave this open for now as we discuss the above. We should also document a behavioral breaking change for 11.1 in this area.

@maxkatz6 maxkatz6 added the by-design The behavior reported in the issue is actually correct. label Jun 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug by-design The behavior reported in the issue is actually correct. enhancement
Projects
None yet
Development

No branches or pull requests

2 participants