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

Can't use Serilog bootstrap logger with JADNC #1082

Closed
LoremFooBar opened this issue Sep 14, 2021 · 4 comments · Fixed by #1431
Closed

Can't use Serilog bootstrap logger with JADNC #1082

LoremFooBar opened this issue Sep 14, 2021 · 4 comments · Fixed by #1431

Comments

@LoremFooBar
Copy link

DESCRIPTION

When trying to implement two-stage logger initialization with Serilog, an exception is thrown:

System.InvalidOperationException: The logger is already frozen.

Exception is not thrown when removing services.AddJsonApi() call. It seems this is happening because the callback to UseSerilog is invoked twice: once when building the ASP.NET host, and once when building an intermediate ServiceProvider in JsonApiApplicationBuilder ctor. as far as I understand, it is recommended to avoid building ServiceProvider during DI services registration.

STEPS TO REPRODUCE

I created a repro: https://github.com/lazyboy1/JsonApiSerilogBootstrapLoggerRepro. Run the project to see the exception.

EXPECTED BEHAVIOR

Exception should not be thrown, and app should start with both the recommended Serilog configuration and JADNC.

ACTUAL BEHAVIOR

Exception is thrown.

VERSIONS USED

  • JsonApiDotNetCore version: 4.2.0
  • ASP.NET Core version: 5.0
  • Entity Framework Core version: 5.0.10
  • Database provider: In memory
@bart-degreed
Copy link
Contributor

Hi @Lazyboy1, thanks for reporting this and providing such a great repro. I've always expected that our intermediate service provider would bite us someday...

The reason we're doing that is two-fold. The first reason is that we're logging some warnings during initialization. This can be fixed by enqueuing those messages and flush them to the logger at a later time. The second reason is more problematic, unfortunately. We need a DbContext instance during initialization because we depend on its .Model property to compose our resource graph. After all that, based on our resource graph, we set up routes and register additional services. So moving all that from AddJsonApi to UseJsonApi makes it impossible to register additional services.

The most reliable/flexible way to obtain a DbContext instance is from the container. I've experimented with some alternative approaches based on https://docs.microsoft.com/en-us/ef/core/dbcontext-configuration/, such as Activator.CreateInstance, but that requires either DbContextOptions<> be passed into AddJsonApi or a parameterless constructor with an OnConfiguring override. But we have cases when users inject extra dependencies into their DbContext, so in general, this is not a reliable solution.

What I found using your repro, is that EF Core obtains a logger from the service container, which triggers the exception in Serilog. After eliminating the first reason mentioned above, combined with preventing EF Core to ask for a logger, the exception disappears, and "Hello World!" appears in the browser.

services.AddDbContext<MyDbContext>(builder =>
{
    builder.UseInMemoryDatabase("db");
    builder.UseLoggerFactory(NullLoggerFactory.Instance);
});

That's the best I can offer, short-term. But this means you won't have any EF Core logging. To fix that, instead of passing a null logger, you may be able to implement a custom factory that somehow waits for Serilog to have initialized before flushing its queue of messages. Or not use the bootstrap logger at all.

I hope that helps a bit. I'm open to suggestions, so if you have any other ideas of how to solve this, please let me know.

@bart-degreed
Copy link
Contributor

@Lazyboy1 Did you get a chance to try the above?

@LoremFooBar
Copy link
Author

Unfortunately not, and it's OK that we close the issue for now. I get that it's not an easy problem to solve.

The easy way around it for us is to not use Serilog's CreateBootstrapLogger, and instead initialize 2 instances of the logger. This approach seems to work, and if it ever breaks we will need to figure something out.

It's also possible to try to get Serilog to be more tolerant in this case. The reason I opened the issue in the first place is because I think Serilog is so widely used, we would want JADNC to be compatible with its best practice, but maybe they are the ones who should be more careful not to break the ecosystem :)

@bart-degreed
Copy link
Contributor

Thanks for understanding and for reporting this. I'm closing this issue, we can revisit if things change. Users should still be able to find this using an issue search.

bkoelman added a commit that referenced this issue Dec 21, 2023
…s considered an anti-pattern, leading to occasional compatibility issues.

This unblocks use in [Aspire](https://learn.microsoft.com/en-us/dotnet/aspire/) (more specifically, its usage of `AddNpgsqlDataSource`, which throws an `ObjectDisposedException`) and fixes #1082.
bkoelman added a commit that referenced this issue Dec 21, 2023
…s considered an anti-pattern, leading to occasional compatibility issues.

This unblocks use in [Aspire](https://learn.microsoft.com/en-us/dotnet/aspire/) (more specifically, its usage of `AddNpgsqlDataSource`, which throws an `ObjectDisposedException`) and fixes #1082.
bkoelman added a commit that referenced this issue Dec 21, 2023
…s considered an anti-pattern, leading to occasional compatibility issues.

This unblocks use in [Aspire](https://learn.microsoft.com/en-us/dotnet/aspire/) (more specifically, its usage of `AddNpgsqlDataSource`, which throws an `ObjectDisposedException`) and fixes #1082.
bkoelman added a commit that referenced this issue Dec 22, 2023
…s considered an anti-pattern, leading to occasional compatibility issues. (#1431)

This unblocks use in [Aspire](https://learn.microsoft.com/en-us/dotnet/aspire/) (more specifically, its usage of `AddNpgsqlDataSource`, which throws an `ObjectDisposedException`) and fixes #1082.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants