rebus-org / Rebus.ServiceProvider

:bus: Microsoft Extensions Dependency Injection container adapter for Rebus
https://mookid.dk/category/rebus
Other
66 stars 32 forks source link

Other background services dependent upon IBus may fail to be created #61

Closed AYss closed 2 years ago

AYss commented 2 years ago

Hi

Rebus7 is started by a background service. The IBus registration is now made as a call to GetBus method of RebusResolver. The resolver gets a bus from DefaultBusInstance (when not in message context). This will likely cause exceptions when there are other background services within the host which require IBus to do publish and/or other stuff. I know no way to initialize background services in some particular order, so the Rebus one starts first and assigns DefaultBusInstance.Bus property... besides it doesn't seem like right approach. What is the suggested solution in such case?

mookid8000 commented 2 years ago

I assume this issue is about Rebus.ServiceProvider, so I am moving it.

Rebus7 is started by a background service. The IBus registration is now made as a call to GetBus method of RebusResolver. The resolver gets a bus from DefaultBusInstance (when not in message context). This will likely cause exceptions when there are other background services within the host which require IBus to do publish and/or other stuff.

I don't understand why it would cause an exception?

AYss commented 2 years ago

Because of this instruction in RebusResolver: var busToReturn = defaultBusInstance.Bus ?? throw new InvalidOperationException. The defaultBusInstance.Bus is null until it is assigned in ExecuteAsync method in RebusBackgroundService.

mookid8000 commented 2 years ago

Oh I understand. Let me just check it out and see what I can do. BRB

mookid8000 commented 2 years ago

I've now moved the bus initialization to a Lazy<Task<>>, which is then used to retrieve the default bus instance. This makes the initialization code oblivious to whether it was called from the background service hosting the bus instance, or from the service provider resolving IBus e.g. to inject it into the constructor of another background service.

It's available on NuGet.org now as Rebus.ServiceProvider.8.0.0-b05 - could you try and see if it fixes your problem? 🙂

AYss commented 2 years ago

The call to resolve IBus still goes to p => p.GetRequiredService<RebusResolver>().GetBus(p). RebusResolver evaluates that messageContext is null so it executes this var defaultBusInstance = serviceProvider.GetService<DefaultBusInstance>(). Yeah we have a DefaultBusInstance. Next we snatch its Bus property, which now points to _instances.Value.Item1 so a private field of the object. That field still happens to be NULL, because its value is set now by a constructor of RebusBackgroundService. But the constructor still haven't been called.

If you force me I'll try make a repro ;). It ain't gonna be easy because the problem occures only on one of our fastest build agents and honestly I don't know how to repro it on my computer on which it works like a charm.

mookid8000 commented 2 years ago

Hmm I reproduced the issue here: https://github.com/rebus-org/Rebus.ServiceProvider/blob/master/Rebus.ServiceProvider.Tests/Bugs/InjectBusIntoAnotherBackgroundService.cs

I must have missed something 😐

AYss commented 2 years ago

Solved it. Looks a bit ugly but works. I've created a wrapper class for the bus instance containing the Lazy<Task<(IBus, BusLifetimeEvents)>> and GetLazyInitializer method with the starting code. Takes all the params that RebusBackgroundService did. RebusBackgroundService takes only RebusInitializer (and Lifetime). Then changed the code in AddRebus like this:

        if (isDefaultBus)
        {
            if (services.Any(s => s.ServiceType == typeof(DefaultBusInstance)))
            {
                throw new InvalidOperationException("Detected that the service collection already contains a default bus registration - please make only one single AddRebus call with isDefaultBus:true");
            }

            services.AddSingleton(p => new RebusInitializer(startAutomatically, key, configure, onCreated, p, isDefaultBus, p.GetService<IHostApplicationLifetime>()));
            services.AddSingleton(p =>
                                  {
                                      var defaultBusInstance = new DefaultBusInstance();
                                      defaultBusInstance.SetInstanceResolver(p.GetRequiredService<RebusInitializer>().BusAndEvents);
                                      return defaultBusInstance;
                                  });
            services.AddSingleton<IHostedService>(p => new RebusBackgroundService(p.GetRequiredService<RebusInitializer>(), p.GetService<IHostApplicationLifetime>()));
        }
        else
        {
            services.AddSingleton<IHostedService>(p => new RebusBackgroundService(new RebusInitializer(startAutomatically, key, configure, onCreated, p, isDefaultBus, p.GetService<IHostApplicationLifetime>()),
                                                                                  p.GetService<IHostApplicationLifetime>()));
        }

The main difference is the DefaultBusInstance is immediately pointing to the lazy boy property. That surely can be simplified and refactored to something more elegant. I can make a PR (I think... never did that before :) ) or just take the idea and go ahead.

mookid8000 commented 2 years ago

If you can submit your fix as a PR, I would be very grateful 🙂 also, you'd get a mention in the changelog and people can see that you are contributing to open source 🎉

AYss commented 2 years ago

If you can submit your fix as a PR, I would be very grateful 🙂

Will do.