Skip to content

[Robustness] AsyncCommand.Execute — async void with no try/catch propagates exceptions to the WPF dispatcher and can crash the UI #866

@Christophe-Rogiers

Description

@Christophe-Rogiers

Severity: Warning

File: src/Servy.UI/Commands/AsyncCommand.cs, lines 30-51

AsyncCommand is the project's general-purpose IAsyncCommand implementation, used as the binding target for any async command in WPF. The ICommand.Execute entry point is async void and unprotected:

public async void Execute(object? parameter)
{
    await ExecuteAsync(parameter);
}

public async Task ExecuteAsync(object? parameter)
{
    if (!CanExecute(parameter)) return;
    try
    {
        _isExecuting = true;
        RaiseCanExecuteChanged();
        await _execute(parameter);
    }
    finally
    {
        _isExecuting = false;
        RaiseCanExecuteChanged();
    }
}

ExecuteAsync has try/finally but no catch. If _execute(parameter) throws — which is the rule, not the exception, for service-control commands hitting SCM, the file system, or the network — the exception:

  1. Surfaces from await _execute(parameter),
  2. Skips through finally (which only resets the flag),
  3. Re-throws from ExecuteAsync,
  4. Becomes an unobserved exception on an async void in Execute(...),
  5. Posts to the synchronization context → Dispatcher.UnhandledException in WPF.

WPF treats Dispatcher.UnhandledException as fatal unless e.Handled = true is set in a global handler. So a user clicking "Start service" while SCM is briefly unreachable risks crashing the Manager / Servy UI instead of seeing a friendly error.

Suggested fix:

The idiomatic AsyncCommand pattern is to surface the exception through Execute (async void) into a controlled handler, while leaving ExecuteAsync re-throwing for callers who explicitly await it:

public async void Execute(object? parameter)
{
    try
    {
        await ExecuteAsync(parameter);
    }
    catch (Exception ex)
    {
        // Log and surface — never let an async void exception escape into the dispatcher.
        Logger.Error(\"AsyncCommand execution failed.\", ex);

        // Optionally re-raise via a configurable error handler so the consuming
        // ViewModel can show a MessageBox without the framework crashing.
        _onError?.Invoke(ex);
    }
}

A second option: keep ExecuteAsync as-is and add catch (Exception ex) { Logger.Error(...); } directly inside ExecuteAsync before the existing finally. Either way, the async void boundary should never let exceptions reach the dispatcher unhandled.

Bonus check: a quick grep across the repo (async void) shows OnProcessExited, CheckHealth, and the two OnTick handlers all wrap their bodies in try/catch — only AsyncCommand.Execute is unprotected.

Metadata

Metadata

Assignees

Labels

enhancementNew feature or request

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions