AddDaprWorkflow causing singletons to be created multiple times

[ante-maric]

Expected Behavior

IServiceCollection.BuildServiceProvider() should never ever be called from application code because it can result in more than one copy of singleton services being created.

Actual Behavior

Adding AddDaprWorkflow call while using Serilog bootstrap logger will throw exception because singleton logger gets created twice:

System.InvalidOperationException: The logger is already frozen.
at Serilog.Extensions.Hosting.ReloadableLogger.Freeze()
at Serilog.SerilogServiceCollectionExtensions.<>c__DisplayClass3_0.b__0(IServiceProvider services)

Freeze method is called only in singleton factory method.

Thrown exception suggests singleton factory method has been called twice.

This is just one symptom that I ran into, but this can cause other issues depending on what singletons an application is using.

Steps to Reproduce the Problem

1. Create new ASP.NET Core Web API project
2. Add Serilog nugets
3. Add Dapr.Workflow nuget
4. In Program.Main create bootstrap logger
   Log.Logger = new LoggerConfiguration()
   .WriteTo.Console()
   .CreateBootstrapLogger();
5. Add dapr workflows: builder.Services.AddDaprWorkflow(x => { });
6. Run the app and observe InvalidOperationException being thrown preventing startup

Root cause can be found here.

Call to BuildServiceProvider() produces warning that someone choose to ignore.

Release Note

RELEASE NOTE:

[WhitWaldo]

Unfortunately, as part of workflow initialization in today's build, we must build a scoped service provider so we might get the registered IHttpClientFactory and current IConfiguration values to provision the factory used to build the Dapr workflow client itself.

Could you simply register your logger after the call to builder.Services.AddDaprWorkflow()? We could modify the documentation to reflect this and potentially add an analyzer to warn about this at build in the future.

It's possible we can revisit changing how the durable task client connects to better expose a more lazily initialized client down the road, but that wouldn't happen until the 1.17 release at the soonest because of other priorities.

[ante-maric]

Unfortunately, order of service registration can not be changed in this case. This bug comes into play only when bootstrap logger is used. Point of bootstrap logger is to be able to start logging even before WebApplicationBuilder is created, so it's created before any of the services can be registered.

One workaround I found is to set preserveStaticLogger to true when calling UseSerilog. It's not ideal, because Serilog configuration that you read using IConfiguration will not be applied to static logger, so catch block in Program.cs will write only to sinks that you hardcode when creating bootstrap logger (which is usually file and console, not very helpful in k8s when pod dies).

If I find the time to create pull request to fix this before v1.17 release, would that be acceptable?

[WhitWaldo]

Certainly - we're looking at releasing 1.16 in the next month, so it's not likely to make it in there, but you're more than welcome to give it a stab for 1.17.

I suspect that this will require changes to the service registration in the upstream dapr/durabletask-dotnet package as well to provide access to an IServiceProvider so it can provision its own IConfiguration lazily, leaving us free to avoid doing that in the scope down in Dapr.Workflow. I'm making a bunch of changes to that package anyway to hammer out some bugs people are experiencing, so I'd certainly urge waiting until post-1.16 to really dig into it, but contributions are certainly welcome.

[ante-maric]

Unfortunately, I couldn't make any time for this. @WhitWaldo will this be on your list for the next release?

[WhitWaldo]

@ante-maric Not likely - 1.17 is going to be a pretty light release given the limited time before the end of the year. Perhaps on 1.18, but again, given how workflow registration currently works with the underlying durabletask library, there'd be a bit of a refactor involved in changing this, so it's not high on the list compared to other nice-to-have items.

[ante-maric]

I probably wouldn’t agree to label this one as nice-to-have, but I do understand there are a ton of other things to tackle.

[WhitWaldo]

I probably wouldn’t agree to label this one as nice-to-have, but I do understand there are a ton of other things to tackle.

I'd be happy with "really nice to have" even, but in terms of weight, I haven't looked at it since I wrote the DI handler in the first place, but I doubt it's a trivial rewrite of how DurableTask is loading it (which is why we opted for this approach with some hesitation in the first place)