Skip to content

Conversation

@yecril71pl
Copy link
Contributor

Mention that the host application lifetime service will be added in addition to the services configured to be run.

Mention that the host application lifetime service will be added *in addition* to the services configured to be run.
@ghost ghost added area-Extensions-Hosting community-contribution Indicates that the PR has been added by a community member labels Dec 24, 2021
@ghost
Copy link

ghost commented Dec 24, 2021

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

Issue Details

Mention that the host application lifetime service will be added in addition to the services configured to be run.

Author: yecril71pl
Assignees: -
Labels:

area-Extensions-Hosting, community-contribution

Milestone: -


/// <summary>
/// Run the given actions to initialize the host. This can only be called once.
/// Adds host application lifetime service to the host.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It adds other services as well, for example logging, configuration, etc. Why are the lifetime services special here?

If we want to list the services added here, I think it would be more appropriate to add them to a <remarks> section instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lifetime services are special because they prevent the application from quitting and you must explicitly ask the application to quit from client code, whereas all other services are used as needed and do not affect the main workflow. That is, when you tell the application to run as a background service, nothing tells you that it will run indefinitely and this is not what you, as a whatever-else programmer, would expect (a whatever-else programmer would expect an explicit call to EnterMainEventLoop or whatever). I tell you this as a whatever-else programmer. Surprise factor 90%.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current worker template has the following code in Program.cs:

using WorkerService6;

IHost host = Host.CreateDefaultBuilder(args)
    .ConfigureServices(services =>
    {
        services.AddHostedService<Worker>();
    })
    .Build();

await host.RunAsync();

The equivalent place to EnterMainEventLoop is the last line: await host.RunAsync();. This is the place where your program will run indefinitely until something cancels it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, and this is unexpected because the code in the Worker provides no indication of an infinite loop and the callback method in Worker is called only once. This is why it looks odd at the first encounter.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The template has a loop in it:

namespace WorkerService6
{
    public class Worker : BackgroundService
    {
        private readonly ILogger<Worker> _logger;

        public Worker(ILogger<Worker> logger)
        {
            _logger = logger;
        }

        protected override async Task ExecuteAsync(CancellationToken stoppingToken)
        {
            while (!stoppingToken.IsCancellationRequested)
            {
                _logger.LogInformation("Worker running at: {time}", DateTimeOffset.Now);
                await Task.Delay(1000, stoppingToken);
            }
        }
    }
}

The idea being that this background task loops until it is done with its work.

Check out https://docs.microsoft.com/en-us/dotnet/core/extensions/workers, which describes how all the pieces fit together.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be fine to add a general statement to the <remarks> section that said something like:

Adds basic services to the host such as application lifetime, host environment, and logging.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The template has a loop in it:

I did not use the template because I did not know it exists; the documentation for BackgroundService does not mention it at all. Do you think it should? I just inherited from BackgroundService, implemented a straightforward procedure and was stupefied by the effect.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be reasonable for the documentation for BackgroundService to point to https://docs.microsoft.com/dotnet/core/extensions/workers.

@gewarren @IEvangelist - do we have any examples of the xml docs linking to the conceptual docs? Here's a case where I think it would be helpful.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not actually sure, but I do agree that would be helpful. Ideally, it would exclude the locale as you've done there. Checkout this grep.app search results.

Copy link
Contributor

@gewarren gewarren Jan 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can add a <related> tag like this, which adds a link under the See also heading in the docs.

yecril71pl and others added 5 commits January 4, 2022 19:03
1. The application will run until interrupted or until `HostApplicationLifetime.StopApplication()` is called.
2. Document how to configure `Services`.
Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great @yecril71pl! Thanks for the contribution, and your patience with my nits.

I just had one small additional comment, and I think this will be ready to merge.

Co-authored-by: Eric Erhardt <[email protected]>
@yecril71pl
Copy link
Contributor Author

This looks great @yecril71pl! Thanks for the contribution, and your patience with my nits.

I just had one small additional comment, and I think this will be ready to merge.

Thank you for helping me to understand how these things work. It all looks like black magic when you see it for the first time.

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks again!

/// the lifetime of the long running operation(s) being performed.
/// </summary>
/// <param name="stoppingToken">Triggered when <see cref="IHostedService.StopAsync(CancellationToken)"/> is called.</param>
/// See <related type="Article" href="/dotnet/core/extensions/workers">Worker Services in .NET</related> for implementation guidelines.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is incorrect, AFAIK. The <related> tag shouldn't have any surrounding text. If you want to have descriptive text like that, I think you need to use a <see href="url">text</see> within the summary or remarks.

yecril71pl and others added 4 commits January 5, 2022 18:30
Co-authored-by: Genevieve Warren <[email protected]>
Co-authored-by: Genevieve Warren <[email protected]>
Co-authored-by: Genevieve Warren <[email protected]>
/// the lifetime of the long running operation(s) being performed.
/// </summary>
/// <param name="stoppingToken">Triggered when <see cref="IHostedService.StopAsync(CancellationToken)"/> is called.</param>
/// See <see href="https://docs.microsoft.com/dotnet/core/extensions/workers">Worker Services in .NET</see> for implementation guidelines.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// See <see href="https://docs.microsoft.com/dotnet/core/extensions/workers">Worker Services in .NET</see> for implementation guidelines.
/// <related type="Article" href="/dotnet/core/extensions/workers">Worker Services in .NET</related>

To follow the example @gewarren linked to:

/// <related type="Article" href="/dotnet/standard/base-types/standard-numeric-format-strings">Standard Numeric Format Strings</related>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But that is a bare reference, with no information why it is there. I would prefer to have it in context. Please confirm.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you decide to keep the <see> tag, you'll need to put the entire text within the summary or remarks tags.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

@yecril71pl yecril71pl Jan 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, and that is for non-essential references, like when you can safely assume the reader already knows those things. This is not the case here.

Co-authored-by: Eric Erhardt <[email protected]>
@eerhardt eerhardt merged commit 74dc242 into dotnet:main Jan 11, 2022
@yecril71pl yecril71pl deleted the patch-3 branch January 11, 2022 22:43
@ghost ghost locked as resolved and limited conversation to collaborators Feb 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-Extensions-Hosting community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants