CanCloseAsync does not work properly with WindowManager

[hoangcuongvn]

I create a sample to reproduce the issue. You will see MyDialogViewModel has its CanCloseAsync returns false but its view/dialog is still closed.
https://drive.google.com/open?id=1e9Qz_iFfZkTpYe2jhlBYZm6vHzkn9f9L

This issue is related to the Window.Closing event which does not work properly with async. I found a workaround.
https://stackoverflow.com/questions/49012892/cannot-use-async-in-closing-method

I suggest to change the WindowManager.Closing method as below

private async void Closing(object sender, CancelEventArgs e)
{
    if (e.Cancel)
    {
        return;
    }

    e.Cancel = true;

    var dialogResult = view.DialogResult;
    var guard = (IGuardClose)model;
    var canClose = await guard.CanCloseAsync(CancellationToken.None);
    if (canClose)
    {
        view.Closing -= Closing;
        if (dialogResult == null)
        {
            view.Close();
        }
        else
        {
            view.DialogResult = dialogResult;
        }
    }
}

[nigel-sampson]

Thanks for the bug report, is this from the latest source? I've fixed up one bug like this already.

[hoangcuongvn]

Yes, it is in the latest source.

Caliburn.Micro/src/Caliburn.Micro.Platform/Platforms/net46/WindowManager.cs

Line 389 in 3884e32

private async void Closing(object sender, CancelEventArgs e)

[hoangcuongvn]

After changing the WindowManager.Closing method as I posted above, the CanCloseAsync works. But that causes other odd issues when the dialog is closed. I tried to change WindowManager.Closing as below, now everything seems fine, no odd issue happens. I have no clue why it works ^_^

private async void Closing(object sender, CancelEventArgs e)
{
	if (e.Cancel)
	{
		return;
	}

	e.Cancel = true;

	var dialogResult = view.DialogResult;
	var guard = (IGuardClose)model;
	var canClose = await guard.CanCloseAsync(CancellationToken.None);
	if (canClose)
	{
		view.Closing -= Closing;

		Execute.BeginOnUIThread(() =>
		{
			if (dialogResult == null)
			{
				view.Close();
			}
			else
			{
				if (view.DialogResult != dialogResult)
				{
					view.DialogResult = dialogResult;
				}
			}
		});
	}
}

[hoangcuongvn]

I found the reason that causes weird errors when the Closing event uses async. It is the Screen.TryCloseAsync method.

Caliburn.Micro/src/Caliburn.Micro.Core/Screen.cs

Line 160 in 3884e32

public virtual Task TryCloseAsync(bool? dialogResult = null)

If I modify it as below, the errors are gone

public virtual Task TryCloseAsync(bool? dialogResult = null)
{
    var closeAction = PlatformProvider.Current.GetViewCloseAction(this, Views.Values, dialogResult);
    return closeAction(CancellationToken.None);
}

The WindowManager.Closing is now like below. Please notice await Task.Yield(), it helps to resolve some strange issues too.

private async void Closing(object sender, CancelEventArgs e)
{
    if (e.Cancel)
    {
        return;
    }

    var dialogResult = view.DialogResult;
    e.Cancel = true;
    await Task.Yield();

    var guard = (IGuardClose)model;
    var canClose = await guard.CanCloseAsync(CancellationToken.None);
    if (canClose)
    {
        view.Closing -= Closing;

        if (dialogResult == null)
        {
            view.Close();
        }
        else
        {
            if (view.DialogResult != dialogResult)
            {
                view.DialogResult = dialogResult;
            }
        }
    }
}

[nigel-sampson]

Pushed some changes to WindowManager that fixes my copy of your example (once CanCloseAsync returns true).

Would love you to check.

[hoangcuongvn]

I try the newest code and I see the exception below when TryCloseAsync(false) is called. It is strange that TryCloseAsync(true) works fine. I attach the sample project underneath. The test code is in MyDialogViewModel.cs.

System.InvalidOperationException
  HResult=0x80131509
  Message=DialogResult can be set only after Window is created and shown as dialog.
  Source=PresentationFramework
  StackTrace:
   at System.Windows.Window.set_DialogResult(Nullable`1 value)
   at Caliburn.Micro.WindowManager.WindowConductor.<Closing>d__9.MoveNext() in d:\project\SMARTSCHEDULER\6.0\SOURCE\_test\Caliburn.Micro\src\Caliburn.Micro.Platform\Platforms\net46\WindowManager.cs:line 416

Test.Wpf.zip

[nigel-sampson]

Thanks, I'll take a look, assume the behavior of the dialog is correct?

[nigel-sampson]

This should now be resolved.

[hoangcuongvn]

After updating the newest code, I see the exception does not happen in the sample above (Test.Wpf.zip).

But if I change the CanCloseAsync as below, another exception happens again.

public override Task<bool> CanCloseAsync(CancellationToken cancellationToken)
{
    return Task.FromResult(true);
}

This error can be resolved by putting await Task.Yield() right after the guard.CanCloseAsync call. I also recommend that not calling view.Close() if cachedDialogResult != null. So the final code is something like:

private async void Closing(object sender, CancelEventArgs e)
{
    if (e.Cancel)
    {
        return;
    }

    var guard = (IGuardClose)model;

    if (actuallyClosing)
    {
        actuallyClosing = false;
        return;
    }

    var cachedDialogResult = view.DialogResult;

    e.Cancel = true;

    var canClose = await guard.CanCloseAsync(CancellationToken.None);
    await Task.Yield();

    if (canClose)
    {
        actuallyClosing = true;

        if (cachedDialogResult == null)
        {
            view.Close();
        }
        else
        {
            if (view.DialogResult != cachedDialogResult)
            {
                view.DialogResult = cachedDialogResult;
            }
        }
    }
}

[nigel-sampson]

#596 has been submitted to fix this in a different way, will look over possible solutions soon.

[nigel-sampson]

Merged #596 to resolve this issue.

[hoangcuongvn]

I updated the newest source and tested with the project in Test.Wpf.zip above. The CanCloseAsync is like below

public override async Task<bool> CanCloseAsync(CancellationToken cancellationToken)
{
    await Task.Delay(1000);
    return true;
}

I see the Accept and Cancel buttons work as expected. But the problem comes when the user closes the dialog by clicking the close icon [x] on the title bar or pressing Alt+F4. In this case the user has to click or press TWICE to close the dialog. The first click or press has no effect. It seems that is due to e.Cancel = false is called TWICE.

I note that await Task.Yield() is not a delay or a hack. It is just a trick to force the Closing method return before the code block if (canClose) { ... } runs (in the case that CanCloseAsync is not a real async method). By other words it forces the Closing method run in a right logic. Personally I see calling e.Cancel = false TWICE is a hack and it in fact does not work perfectly.

[nigel-sampson]

I realise what it does, and yes the current behavior isn't ideal (and will be fixed).

[nigel-sampson]

Pushed an approach using Task.Yield, my comments about this method were based on most other times I've seen it used to paper over people not understanding UWP and the UI thread.

[nigel-sampson]

Assume this is resolved @hoangcuongvn ?

[hoangcuongvn]

Nigel, this issue still exists. If CanCloseAsync simply returns true as return Task.FromResult(true), an exception happens when the user clicks on the Cancel button (in my above sample project).

This issue can be resolved if the Screen.TryCloseAsync is modified as below:

public virtual async Task TryCloseAsync(bool? dialogResult = null)
{
    if (Parent is IConductor conductor)
    {
        await conductor.CloseItemAsync(this, CancellationToken.None);
    }

    var closeAction = PlatformProvider.Current.GetViewCloseAction(this, Views.Values, dialogResult);
    await closeAction(CancellationToken.None);
}

[nigel-sampson]

I can't recreate this issue on my machine using your example.

public class MyDialogViewModel : Screen
{
    public async void DoAccept()
    {
        await TryCloseAsync(true);
    }

    public async void DoCancel()
    {
        await TryCloseAsync(false);
    }

    public override Task<bool> CanCloseAsync(CancellationToken cancellationToken)
    {
        return Task.FromResult(true);
    }
}

[hoangcuongvn]

Your code above is correct. The issue only happens when the user clicks on the Cancel button. I record a video of reproducing the exception in the link below:
https://youtu.be/g62aKKaCty4

[nigel-sampson]

I still can't; recreate this problem with the current version of the code.

[hoangcuongvn]

It is quite strange. I am sure that I was using the latest source code.

Please have a look in this new video to check if anything is missed?
https://youtu.be/M_Kj1_87owo

[nigel-sampson]

Nothing stands out as wrong, you're clearly rebuilding the source which was main thing I was looking for.

I'm curious if you have the latest version, the commit c656c51 has a fix for Execute.OnUIThreadAsync, given the nature of your fix it's possible that latest commit isn't on your machine.

[hoangcuongvn]

Yes, the source I was using is the latest and it does include the c656c51 fix.

[nigel-sampson]

Not sure what's going on then. Will take another look at it once I'm back from Easter break.

[hoangcuongvn]

Hi Nigel, have you re-checked this issue? I am using CM in my real Wpf projects. In simple projects which have less UI interaction, the error does not happen. But in more complicated projects with more UI interaction, this error happens. If I modify the Screen.TryCloseAsync as I posted above, the error no longer happens.

[nigel-sampson]

I haven't been able to recreate the issue with the current version of the code.

[hoangcuongvn]

I created a full solution in the link below. Can you download and run it? The dialog has 2 button Accept and Cancel. Please click on the Cancel button. Does the error happen?
https://drive.google.com/open?id=1LqFTzKF8vaVVcYpyj-rqggAWd-1UPeGP