Skip to content

[Correctness] Service.cs — OnProcessExited awaits semaphore outside try/catch in async void #789

@Christophe-Rogiers

Description

@Christophe-Rogiers

Severity: Warning
File: src/Servy.Service/Service.cs lines 1531-1546

Description:

OnProcessExited is the Exited-event handler and is declared async void (required for event-handler signatures). The current body awaits _healthCheckSemaphore.WaitAsync() before the try block starts:

private async void OnProcessExited(object sender, EventArgs e)
{
    if (_isTearingDown || _disposed) return;

    _logger?.Warn("Child process exit detected via event.");
    ClearProcessState();

    bool needsRecovery = false;
    bool shouldStop = false;
    int exitCode = -1;

    // Use WaitAsync instead of Wait to avoid blocking thread pool threads
    await _healthCheckSemaphore.WaitAsync();   // ← line 1543, OUTSIDE try

    try
    {
        try
        {
            exitCode = _childProcess?.ExitCode ?? -1;
        }
        ...

#473 (closed) correctly moved this from blocking Wait() to WaitAsync(), but left the await outside the exception-handling boundary.

Failure mode: the (_isTearingDown || _disposed) check on line 1533 is not synchronized with teardown — between that check and the await on line 1543, ExecuteTeardown running on another thread can Dispose() the semaphore. WaitAsync() then throws ObjectDisposedException. Because the method is async void, the exception unwinds onto the ambient SynchronizationContext (none, so the default TaskScheduler) and surfaces as an unhandled TaskScheduler.UnobservedTaskException / crash, rather than being logged by the method's own catch block further down.

It is a narrow race — you need a child-exit event to fire during teardown — but in a Windows Service the shutdown path is exactly when child-exit events are most likely (SCM stopping → child tree collapses).

Suggested fix:

Move the await inside the outer try so the existing catch handler covers it:

private async void OnProcessExited(object sender, EventArgs e)
{
    try
    {
        if (_isTearingDown || _disposed) return;

        _logger?.Warn("Child process exit detected via event.");
        ClearProcessState();

        bool needsRecovery = false;
        bool shouldStop = false;
        int exitCode = -1;

        await _healthCheckSemaphore.WaitAsync();
        try
        {
            ...
        }
        finally
        {
            _healthCheckSemaphore.Release();
        }
    }
    catch (ObjectDisposedException)
    {
        // Teardown raced us; nothing to do.
    }
    catch (Exception ex)
    {
        _logger?.Error($"OnProcessExited failed: {ex.Message}", ex);
    }
}

Metadata

Metadata

Assignees

Labels

bugSomething isn't working

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions