BrighterCommand / Brighter

A framework for building messaging apps with .NET and C#.
https://www.goparamore.io/
MIT License
2.03k stars 257 forks source link

Issue after upgrading from 8.x to 9.x #2664

Closed provanguard closed 1 year ago

provanguard commented 1 year ago

Describe the bug

I have a web app that uses Brighter. It also has Hangfire and MongoDB as backend. Hangfire jobs call Brighter commands. After upgrading to Brighter 9.x, when running in Azure, the web app gets an error where it effectively cannot execute any Hangfire jobs.

Exceptions (if any)

exception.innerException.innerException.Message Cannot access a disposed object. Object name: 'IServiceProvider'.

exception.innerException.innerException.Source Microsoft.Extensions.DependencyInjection

exception.innerException.innerException.StackTraceString at Microsoft.Extensions.DependencyInjection.ServiceLookup.ThrowHelper.ThrowObjectDisposedException() at Microsoft.Extensions.DependencyInjection.ServiceLookup.ServiceProviderEngineScope.GetService(Type serviceType) at Paramore.Brighter.Extensions.DependencyInjection.ServiceProviderHandlerFactory.Paramore.Brighter.IAmAHandlerFactoryAsync.Create(Type handlerType) in /_/src/Paramore.Brighter.Extensions.DependencyInjection/ServiceProviderHandlerFactory.cs:line 68 at Paramore.Brighter.Interpreter1.<GetAsyncHandlers>b__7_0(Type handlerType) in /_/src/Paramore.Brighter/Interpreter.cs:line 63 at System.Linq.Enumerable.SelectListIterator2.MoveNext() at System.Linq.Enumerable.CastIterator[TResult](IEnumerable source)+MoveNext() at System.Linq.Enumerable.CastIterator[TResult](IEnumerable source)+MoveNext() at Paramore.Brighter.Extensions.EnumerationExtensions.Each[T](IEnumerable1 collection, Action1 doThis) in //src/Paramore.Brighter/Extensions/Each.cs:line 41 at Paramore.Brighter.PipelineBuilder`1.BuildAsync(IRequestContext requestContext, Boolean continueOnCapturedContext) in //src/Paramore.Brighter/PipelineBuilder.cs:line 173

exception.innerException.Message Error when building pipeline, see inner Exception for details

exception.innerException.Source Paramore.Brighter

exception.innerException.StackTraceString at Paramore.Brighter.PipelineBuilder`1.BuildAsync(IRequestContext requestContext, Boolean continueOnCapturedContext) in //src/Paramore.Brighter/PipelineBuilder.cs:line 173 at Paramore.Brighter.CommandProcessor.SendAsync[T](T command, Boolean continueOnCapturedContext, CancellationToken cancellationToken) in //src/Paramore.Brighter/CommandProcessor.cs:line 356 at Tx.Ds.Dc.Ports.Commands.SMCommandHandlerAsync.HandleAsync(SMJobCommand command, CancellationToken cancellationToken) in /home/vsts/work/1/s/src/DC/Ports/Commands/SMJobCommand.cs:line 74 at Paramore.Brighter.CommandProcessor.SendAsync[T](T command, Boolean continueOnCapturedContext, CancellationToken cancellationToken) in /_/src/Paramore.Brighter/CommandProcessor.cs:line 356

exception.Message A Task's exception(s) were not observed either by Waiting on the Task or accessing its Exception property. As a result, the unobserved exception was rethrown by the finalizer thread. (Error when building pipeline, see inner Exception for details)

message Unhandled AppDomain Exception in Dp Server

Lines 73 and 74 (cleaned) var myCommand = new CallXCommand(params); await _commandProcessor.SendAsync(myCommand, cancellationToken: cancellationToken);

Further technical details

provanguard commented 1 year ago

The structure of configuration and code is as so:

Startup.cs

private void RegisterHangfireAndJobs(IServiceCollection services) { services.AddHangfire(configuration => { }); services.AddSingleton<IRecurringJob, SMJob>(); services.AddSingleton<JobBootstrapper, JobBootstrapper>(); }

SMJob.cs

public override void Work() { var command = new SMJobCommand(); _commandProcessor.SendAsync(command); }

SMJobCommandHandlerAsync

Lines 73 and 74 (cleaned) var myCommand = new CallXCommand(params); await _commandProcessor.SendAsync(myCommand, cancellationToken: cancellationToken);

Things are working fine locally. The issue happens when running in Azure.

provanguard commented 1 year ago

Please kindly provide some suggestion. I am stuck with version 8.x for the time being, which is getting old.

holytshirt commented 1 year ago

Hi @provanguard this is very odd, especially if it is working locally. The odd thing is it's saying is IServiceProvider is disposed which is the container but that error is happening in the ServiceProviderEngineScope which might be a clue that a scope IServiceProvider is disposed already. In V9 one of the biggest issues we had to deal with is EF Core + aspnetcore and scopes. I see you have registered the handler as Singleton and passing in _commandProcessor which is effectively making _commandProcessor a Singleton. I V8 the default registration of CommandProcessor was Singleton and in V9 it is Transient.

My first recommendation would be to try updating your CommandProcessor registration to Singleton

 services.AddBrighter(options =>
                {
                    options.CommandProcessorLifetime = ServiceLifetime.Singleton;
                })

an example of controlling the lifetimes is here https://github.com/BrighterCommand/Brighter/blob/master/samples/WebAPI_Dapper/GreetingsWeb/Startup.cs#L164-L171

My second recommendation based on you saying it works locally but not in azure is that the dotnet container works differently in development mode vs production https://learn.microsoft.com/en-us/dotnet/core/extensions/dependency-injection#scope-validation So try changing Development in your launchSettings.json to Production and see if you get error.

I hope that helps and let us know how you get on.

provanguard commented 1 year ago

I will give it a try and report back on Monday (bank holiday starting now). I hope that just solves it :)

provanguard commented 1 year ago

I have been running the latest 9.x in dev and prod in Azure for about an hour already, with the "options.CommandProcessorLifetime = ServiceLifetime.Singleton;" setting, and so far it is working! I know it is a bit early to conclude that everything works 100% after 1 hour, but so far so good. The original issue is resolved.

I have few follow up questions on this.

  1. What side effects should I expect when running 9.x with that non-default setting?
  2. One breaking change that I had to modify my code for 9.x to work, was the multiple policies configuration. I have 4 commands that used to have 2 separate policies configured like this before:

[UsePolicyAsync(policy: CommandProcessor.CIRCUITBREAKERASYNC, step: 1)] [UsePolicyAsync(policy: CommandProcessor.RETRYPOLICYASYNC, step: 2)]

With the new version I use this syntax: [UsePolicyAsync(new[] { CommandProcessor.CIRCUITBREAKERASYNC, CommandProcessor.RETRYPOLICYASYNC }, 1)]

Could you confirm that the syntax is correct and that the order of the listed policies is correct? I added these policies about 5 years ago and at the time I believe I did the research and testing on it. It might have been based on the code samples available at the time. Please let me know if this approach is still the way to go, or is it perhaps superfluous?

iancooper commented 1 year ago

@provanguard Missed this but for reference this is correct