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

Validate StartAsync() exception semantics for Starting, Start, Started #88603

Closed
steveharter opened this issue Jul 10, 2023 · 6 comments · Fixed by #90183
Closed

Validate StartAsync() exception semantics for Starting, Start, Started #88603

steveharter opened this issue Jul 10, 2023 · 6 comments · Fixed by #90183
Assignees
Milestone

Comments

@steveharter
Copy link
Member

steveharter commented Jul 10, 2023

As part of #87335 and #88546 we should evaluate the current exception handling abort semantics between the stages which is currently:

  • An exception in any handlers of Starting, Start and Started do not prevent the next stage from running.
    • Exceptions are gathered, the next stage is run and after Started is run, an exception is thrown and IHostApplicationLifetime.ApplicationStarted is not called.
  • Within each stage, if an exception is thrown:
    • If non-concurrent, the rest of the stage is cancelled.
    • If concurrent, the rest of the stage continues (since they are all run concurrently). Note that concurrent mode is new for v8.
    • These semantics were not changed with the above PRs.

For the current implementation, see StartAsync() here.

Proposal: If there is an exception in Starting, Start or Started, processing stops and the next stage is not run. The exception is logged and re-thrown. For Started, if there is an exception then IHostApplicationLifetime.ApplicationStarted is not run which is consistent with the prior behavior (before the new stages were added in #87335) if there was an exception in Start.

Note that for Stopping\Stop\Stopped, it makes sense to continue to run each phase so that the proper clean-up work is handled, so this does not have the same semantics as Starting\Start\Starting. As a side note, we should verify when ASP.NET calls StopAsync() if there is an exception in StartAsync().

@steveharter steveharter added this to the 8.0.0 milestone Jul 10, 2023
@steveharter steveharter self-assigned this Jul 10, 2023
@steveharter
Copy link
Member Author

@ghost
Copy link

ghost commented Jul 10, 2023

Tagging subscribers to this area: @dotnet/area-extensions-hosting
See info in area-owners.md if you want to be subscribed.

Issue Details

As part of #87335 and #88546 we should evaluate the current exception handling abort semantics between the stages which is currently:

  • An exception in any handlers of Starting, Start and Started do not prevent the next stage from running.
    • Exceptions are gathered, the next stage is run and after Started is run, an exception is thrown and IHostApplicationLifetime.ApplicationStarted is not called.
  • Within each stage, if an exception is thrown:
    • If non-concurrent, the rest of the stage is cancelled.
    • If concurrent, the rest of the stage continues (since they are all run concurrently). Note that concurrent mode is new for v8.
    • These semantics were not changed with the above PRs.

For the current implementation, see StartAsync() here.

Proposal:

  • If there is an exception in Starting, Start or Started, processing stops and Start is not run. The exception is raised, logged, and re-thrown.
    • If there is an exception in Started,IHostApplicationLifetime.ApplicationStarted is not run. This is consistent with the prior behavior before Add IHostedLifecycleService #87335 if there was an exception in Started.

Note that for Stopping\Stop\Stopped, it makes sense to continue to run each phase so that the proper clean-up work is handled, so this does not have the same semantics as Starting\Start\Starting. As a side note, we should verify when ASP.NET calls StopAsync() if there is an exception in StartAsync().

Author: steveharter
Assignees: steveharter
Labels:

area-Extensions-Hosting

Milestone: 8.0.0

@stevejgordon
Copy link
Contributor

@steveharter Apologies, as this is not directly related to the issue above, but I didn't feel it warranted a new issue, and the original PR is locked.

I'm writing a blog post on the new lifecycle phases added with IHostedLifecycleService. One thing I'm missing is the reason for the Task.Run call here. When invoking the delegate here, the tasks are already scheduled/running, so could those not simply be stored into the list to be awaited with WhenAll? What is the benefit provided by wrapping them in a proxy task?

@steveharter
Copy link
Member Author

One thing I'm missing is the reason for the Task.Run call here. When invoking the delegate here, the tasks are already scheduled/running, so could those not simply be stored into the list to be awaited with WhenAll? What is the benefit provided by wrapping them in a proxy task?

For the case when Task.Run() is added to the List<Task>, the task was called synchronously but it returned since it hit an await and is no longer running. Using Task.Run() forces the rest of the task to run concurrently with other tasks instead of using the synchronization context \ scheduler which may or may not run the tasks concurrently if just WhenAll is used. Note the use of Task.Run() was done in a previous PR that enabled concurrent mode.

@stevejgordon
Copy link
Contributor

Thanks for responding with this answer. I'm not sure I fully follow though and perhaps there's nuance I'm not understanding. As the host is used in console apps, there is no specific synchronization context, so everything ends up scheduled for the ThreadPool. While the methods run synchronously until the first await, the work is then (likely immediately) scheduled onto a thread pool thread and running concurrently, even before the Task.Run. The returned tasks could then simply be awaited. For example this code....

Console.WriteLine($"Started app on thread {Environment.CurrentManagedThreadId}");

List<Task> tasks = new();

var ops = new Func<Task>[]
{
    Test.TaskOne,
    Test.TaskTwo,
    Test.TaskThree,
    Test.TaskFour,
    Test.TaskFive
};

for (var i = 0; i < ops.Length; i++)
{
    Console.WriteLine($"Invoking Task {i + 1} on thread {Environment.CurrentManagedThreadId}.");
    var task = ops[i]();

    if (task.IsCompleted)
    {
        Console.WriteLine($"Task {i + 1} on thread {Environment.CurrentManagedThreadId} is completed synchronously.");
    }
    else
    {
        Console.WriteLine($"Task {i + 1} running asynchronously.");
        tasks.Add(task);
    }
}

Console.WriteLine($"Waiting for keypress.");
Console.ReadKey();
Console.WriteLine($"Calling WhenAll.");
await Task.WhenAll(tasks).ConfigureAwait(false);
Console.WriteLine($"Finished app on thread {Environment.CurrentManagedThreadId}.");

public static class Test
{
    public static Task TaskOne()
    {
        Console.WriteLine($"TASK ONE: Starting on thread {Environment.CurrentManagedThreadId}.");
        return Task.CompletedTask;
    }

    public static async Task TaskTwo()
    {
        Console.WriteLine($"TASK TWO: Starting on thread {Environment.CurrentManagedThreadId}.");
        await Task.Yield();
        Console.WriteLine($"TASK TWO: After Yield on thread {Environment.CurrentManagedThreadId}.");
        await Task.Delay(2000);
        Console.WriteLine($"TASK TWO: Completed after 2s delay on thread {Environment.CurrentManagedThreadId}.");
    }

    public static async Task TaskThree()
    {
        Console.WriteLine($"TASK THREE: Starting on thread {Environment.CurrentManagedThreadId}.");
        await Task.Yield();
        Console.WriteLine($"TASK THREE: After Yield on thread {Environment.CurrentManagedThreadId}.");
        await Task.Delay(1000);
        Console.WriteLine($"TASK THREE: Completed after 1s delay on thread {Environment.CurrentManagedThreadId}.");    
    }

    public static Task TaskFour()
    {
        Console.WriteLine($"TASK FOUR: Starting on thread {Environment.CurrentManagedThreadId}.");
        return Task.CompletedTask;
    }

    public static async Task TaskFive()
    {
        Console.WriteLine($"TASK FIVE: Starting on thread {Environment.CurrentManagedThreadId}.");
        await Task.Yield();
        Console.WriteLine($"TASK FIVE: After Yield on thread {Environment.CurrentManagedThreadId}.");
        await Task.Delay(5000);
        Console.WriteLine($"TASK FIVE: Completed after 5s delay on thread {Environment.CurrentManagedThreadId}.");
    }
}

runs generally as follows:

Started app on thread 1
Invoking Task 1 on thread 1.
TASK ONE: Starting on thread 1.
Task 1 on thread 1 is completed synchronously.
Invoking Task 2 on thread 1.
TASK TWO: Starting on thread 1.
Task 2 running asynchronously.
Invoking Task 3 on thread 1.
TASK TWO: After Yield on thread 4.
TASK THREE: Starting on thread 1.
Task 3 running asynchronously.
Invoking Task 4 on thread 1.
TASK FOUR: Starting on thread 1.
Task 4 on thread 1 is completed synchronously.
Invoking Task 5 on thread 1.
TASK THREE: After Yield on thread 7.
TASK FIVE: Starting on thread 1.
Task 5 running asynchronously.
Waiting for keypress.
TASK FIVE: After Yield on thread 7.
TASK THREE: Completed after 1s delay on thread 6.
TASK TWO: Completed after 2s delay on thread 6.
TASK FIVE: Completed after 5s delay on thread 6.
Calling WhenAll.
Finished app on thread 1.

@steveharter
Copy link
Member Author

Thanks for responding with this answer. I'm not sure I fully follow though and perhaps there's nuance I'm not understanding. As the host is used in console app

The host is generic - can be hosted by console app, ASP.NET, service, etc where they can have a custom task scheduler.

However perhaps the use of Task.Run() is overkill. Thoughts @stephentoub?

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Aug 8, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Aug 9, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Sep 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants