SteeltoeOSS / Steeltoe

.NET Components for Externalized Configuration, Database Connectors, Service Discovery, Logging and Distributed Tracing, Application Management, Security, and more.
https://steeltoe.io
Apache License 2.0
1.01k stars 163 forks source link

Asynchronous and Independent ApplicationTask #1328

Closed bdcberuni closed 3 months ago

bdcberuni commented 4 months ago

Is your feature request related to a problem? Please describe.

Currently, the IApplicationTask exposes a sync method Run which doesn't allow us to properly do asynchounous operations without having to do GetAwaiter().GetResult() on those operations. Since all Application Tasks are registered and retrieved from an IEnumerable, they all need to be initialized for one to be executed. This can cause issue if one Application Task have a dependency that is not currently available (e.g. Database) but is not related to the one that was asked to run.

Describe the solution you'd like

I would like to be able to:

  1. Implement the IApplicationTask with an async operation inside the Run.
  2. Run a task without having to build all IApplicationTask registered in the DI.

Describe alternatives you've considered

We have changed the IApplicationTask with the Run method to return a Task instead of void We have changed the TaskServiceCollectionExtensions to register the IApplicationTask as a Keyed Service We have changed the TaskWebHostExtensions to get a keyed service instead of an IEnumerable<IApplicationTask>

bart-vmware commented 4 months ago

Hi @bdcberuni, thanks for your proposal.

  1. Seems reasonable, we should make the existing Run method async and take a CancellationToken.
  2. Why is this a problem? Wouldn't initialization typically execute from the Run method instead of the IApplicationTask constructor?
bdcberuni commented 4 months ago

Since the FindAndRunTask get all the IApplicationTask registered in the DI, they must all be built. Which means that all dependencies needed in their constructors will also be built. The workaround would be to use the IServiceProvider in the constructor of each Tasks, then get the service needed when the Run is called. But that's using the service locator pattern, which is not that great.

bart-vmware commented 3 months ago

That makes sense, I understand now. Using keyed services solves that problem.

These are great suggestions. I've implemented them in #1331, which will ship with Steeltoe v4.x.

Can you please verify if the new API satisfies your needs?

bdcberuni commented 3 months ago

The changes to the interface satisfies my needs!

I have maybe another request. In the TaskHostExtensions, would it be possible to have those methods as well?

// Executes the specified taskName asynchronously. The host instance is disposed after running
RunTaskAsync(this IHost host, string taskName, CancellationToken token = default);

// Similar to RunTaskAsync, but doesn't dispose the host
StartTaskAsync(this IHost host, string taskName, CancellationToken token = default)

// Similar to RunWithTaskAsync, but calls the Start on the Host instead of the of Run and doesn't dispose the host.
StartWithTasksAsync(this IHost host, CancellationToken token = default)

Note: Since the host.RunAsync disposes the host, I would expect the same behavior upon calling host.RunWithTaskAsync while providing a runTask configuration value. Which I think is not done in your implementation.

bart-vmware commented 3 months ago

Thanks, I didn't realize that RunAsync disposes the host, so I'll add that when a task runs.

From what I understand, the IApplicationTask feature exists for the following reason: If the app starts with a RunTask= command-line parameter, it should execute IApplicationTask.Run instead of the app itself. Steeltoe offers this because it's a Cloud Foundry feature.

Can you clarify what the advantage would be of adding these additional methods to Steeltoe? Which scenarios would it enable that aren't covered already? I don't understand what the added value would be if Steeltoe offered these.

bdcberuni commented 3 months ago

It's probably a low value for the methods RunTask/StartTask. We had some case in our company where we would deploy applications that would only work with a scheduled task so the argument runTask was required (not optional).

Otherwise, thank you for the changes! Looking forward to use v4.X!