.NET8 GetServices issue orleans8

[stephenlautier]

As per the discussion in this thread and specifically using jods4:net8 PR branch

Will open an new issue so not to clutter the other thread

---

During startup Im having the following exception (which is not from our code but from Orleans)

Full stacktrace

Grace.DependencyInjection.Exceptions.LocateException: 'Could not locate Type System.String
1 Importing System.Collections.Generic.IEnumerable`1[Orleans.Runtime.Hosting.NamedService`1[Orleans.GrainDirectory.IGrainDirectory]] 
2 Importing Orleans.Runtime.Hosting.NamedService`1[Orleans.GrainDirectory.IGrainDirectory][] 
3 Importing Orleans.Runtime.Hosting.NamedService`1[Orleans.GrainDirectory.IGrainDirectory] 
4 Importing System.String  for constructor parameter name
'

Alto seeing the code its seems "intended" as its marked as required and no default

Usage in Microsoft Orleans (not our source)

return serviceProvider.GetServices<NamedService<IGrainDirectory>>() ?? Enumerable.Empty<NamedService<IGrainDirectory>>();

https://github.com/dotnet/orleans/blob/main/src/Orleans.Runtime/Hosting/DirectorySiloBuilderExtensions.cs#L71

Registration in Microsoft Orleans (not our source)

UPDATE NOTE: this doesn't get invoked when debugging

        public static IServiceCollection AddGrainDirectory<T>(this IServiceCollection collection, string name, Func<IServiceProvider, string, T> implementationFactory)
            where T : IGrainDirectory
        {
            collection.AddSingleton(sp => new NamedService<IGrainDirectory>(name, implementationFactory(sp, name)));
            // Check if the grain directory implements ILifecycleParticipant<ISiloLifecycle>
            if (typeof(ILifecycleParticipant<ISiloLifecycle>).IsAssignableFrom(typeof(T)))
            {
                collection.AddSingleton(s => (ILifecycleParticipant<ISiloLifecycle>)s.GetGrainDirectory(name));
            }
            return collection;
        }

https://github.com/dotnet/orleans/blob/main/src/Orleans.Runtime/Hosting/DirectorySiloBuilderExtensions.cs#L42

This is our config for grace (though tried to comment the Behaviors but still the same)

var graceConfig = new InjectionScopeConfiguration
{
	Behaviors =
	{
		AllowInstanceAndFactoryToReturnNull = true,
	}
};

Not sure if we need to configure something specifically, or its a bug

To isolate the issue, I added a very basic test to test similar what they are doing however that succeeds

test source

using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.DependencyInjection.Specification;
using System;
using System.Linq;
using Xunit;

namespace Grace.DependencyInjection.Extensions.Tests
{
    public class GraceContainerTests : DependencyInjectionSpecificationTests
    {
        protected override IServiceProvider CreateServiceProvider(IServiceCollection serviceCollection)
        {
            return new DependencyInjectionContainer(x => x.Behaviors.AllowInstanceAndFactoryToReturnNull = true)
                .Populate(serviceCollection);
        }

        [Fact]
        public void GetServices()
        {
            var services = new ServiceCollection()
                    .AddSingleton(sp => new NamedService<IHeroesIndex> { Name = "name", Value = new HeroesIndex() })
                ;
            var providers = CreateServiceProvider(services);

            var q = providers.GetServices<NamedService<IHeroesIndex>>() ?? Enumerable.Empty<NamedService<IHeroesIndex>>();
            Assert.NotEmpty(q);
        }
    }
}

public class NamedService<T>
{
    public string Name { get; set; }
    public T Value { get; set; }
}

public interface IHeroesIndex
{
}

public class HeroesIndex : IHeroesIndex
{

}

Also replicated the issue with a basic Orleans sample (and also isolate it from our stuff)

• Clone https://github.com/stephenlautier/dotnet.samples/blob/feature/grace-sample/orleans/Adventure/README.md
• Link Grace projs to your branch
• Simply run AdventureServer

You should get the above exception on launch

[stephenlautier]

Continue to experiment with tests (alto i didnt replicate the exact issue) im seeing something strange (which could also blow internally) and its inconsistent (default behaviour, no config passed)

// returns 1 single entry when no services are registered with null data (not expected)
var strats = providers.GetServices<NamedService<StrategyIndex>>();// ?? Enumerable.Empty<NamedService<IHeroesIndex>>();

// returns empty when no services are registered (expected)
var stratsKeyed = providers.GetKeyedServices<NamedService<StrategyIndex>>("strat");

[jods4]

The problem is described here:
Importing System.String for constructor parameter name

Because NamedService<> ctor takes a string name parameter, which Grace attempts to inject and fails (kind of expected).

I've done a quick search to check in Orleans code how NamedService is registered in DI but didn't find it, I need more time.
I think it might be this code: https://github.com/dotnet/orleans/blob/4166e7804d82b974b5be32797af27bb68ebe9895/src/Orleans.Core/Configuration/NamedServiceConfigurator.cs#L102-L109
So specific instances of NamedService<T> are registered with a keyed factory that passes the key to the ctor itself.

Your issue would happen when no such service is registered and Grace attempts to locate these on its own anyway, and fails because it doesn't know how to locate string.

It really reminds me of #309, even though you said AutoRegisterUnknown = false doesn't help. Note that #309 is a pre-.net 8, so maybe it has regressed with the .net 8 changes.

Keep in mind I haven't had time to debug, this is just intuition and maybe not everything above is correct.

Just to be sure: you've combined the .net 8 Grace branch with the latest Grace.DependencyInjection.Extensions branch as well?
ipjohnson/Grace.DependencyInjection.Extensions#36

[stephenlautier]

Just to be sure: you've combined the .net 8 Grace branch with the latest Grace.DependencyInjection.Extensions branch as well?
ipjohnson/Grace.DependencyInjection.Extensions#36

Yes, i pulled your branches and built local nugets with those (key services works now). Now added a sample of keyed service + included grace projs in the sample Adventure.sln to be 100% sure

I've done a quick search to check in Orleans code how NamedService is registered in DI but didn't find it, I need more time.
I think it might be this code: https://github.com/dotnet/orleans/blob/4166e7804d82b974b5be32797af27bb68ebe9895/src/Orleans.Core/Configuration/NamedServiceConfigurator.cs#L102-L109

No, not that part, I already posted how they are doing it above, which is this one just in case you missed it:

Registration in Microsoft Orleans (not our source)

UPDATE NOTE: this doesn't get invoked when debugging

        public static IServiceCollection AddGrainDirectory<T>(this IServiceCollection collection, string name, Func<IServiceProvider, string, T> implementationFactory)
            where T : IGrainDirectory
        {
            collection.AddSingleton(sp => new NamedService<IGrainDirectory>(name, implementationFactory(sp, name)));
            // Check if the grain directory implements ILifecycleParticipant<ISiloLifecycle>
            if (typeof(ILifecycleParticipant<ISiloLifecycle>).IsAssignableFrom(typeof(T)))
            {
                collection.AddSingleton(s => (ILifecycleParticipant<ISiloLifecycle>)s.GetGrainDirectory(name));
            }
            return collection;
        }

https://github.com/dotnet/orleans/blob/main/src/Orleans.Runtime/Hosting/DirectorySiloBuilderExtensions.cs#L42

The problem is described here:
Importing System.String for constructor parameter name
Because NamedService<> ctor takes a string name parameter, which Grace attempts to inject and fails (kind of expected).

Thanks!! i was missing the ctor to replicate it, now i managed to isolate it in the unit test 🥳

using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.DependencyInjection.Specification;
using System;
using Xunit;

namespace Grace.DependencyInjection.Extensions.Tests
{
    public class GraceContainerTests : DependencyInjectionSpecificationTests
    {
        protected override IServiceProvider CreateServiceProvider(IServiceCollection serviceCollection)
        {
            void Configuration(InjectionScopeConfiguration x) => x.Behaviors.AllowInstanceAndFactoryToReturnNull = true;

            //return new DependencyInjectionContainer(Configuration)
            return new DependencyInjectionContainer()
                .Populate(serviceCollection);
        }

        [Fact]
        public void GetServices()
        {
            var services = new ServiceCollection()
                //.AddSingleton(sp => new NamedService<StrategyIndex> { Name = "strat1", Value = new StrategyIndex() })
                //.AddSingleton(sp => new NamedService<StrategyIndex> { Name = "strat2", Value = new StrategyIndex() })
                .AddSingleton(sp => new NamedService<IHeroesIndex>("wow", new HeroesIndex()))
                //.AddKeyedSingleton("strat", (sp, k) => new NamedService<StrategyIndex> { Name = "statA", Value = new StrategyIndex() })
                //.AddKeyedSingleton("strat", (sp, k) => new NamedService<StrategyIndex> { Name = "statB", Value = new StrategyIndex() })
                //.AddSingleton(sp => new NamedService<IHeroesIndex> { Name = "name", Value = new HeroesIndex() })
                ;
            var providers = CreateServiceProvider(services);

            var q = providers.GetServices<NamedService<IHeroesIndex>>();// ?? Enumerable.Empty<NamedService<IHeroesIndex>>();
            Assert.NotEmpty(q);

            // returns 1 single entry when no services are registered with null data (not expected)
            var strats = providers.GetServices<NamedService<StrategyIndex>>();// ?? Enumerable.Empty<NamedService<IHeroesIndex>>();

            // returns empty when no services are registered (expected)
            var stratsKeyed = providers.GetKeyedServices<NamedService<StrategyIndex>>("strat");
        }
    }
}

public class NamedService<TService>
{
    public NamedService(string name, TService service)
    {
        Name = name;
        Service = service;
    }


    public string Name { get; }
    public TService Service { get; }
}

public interface IHeroesIndex
{
}

public class HeroesIndex : IHeroesIndex
{

}
public class StrategyIndex
{

}

[jods4]

collection.AddSingleton(sp => new NamedService<IGrainDirectory>(name, implementationFactory(sp, name)));

Ok thanks for finding that code for me!
If this was called, it's a simple singleton instance provided at registration, Grace would never call the NamedService ctor.
It would be a regression in my net8 branch if that didn't work.

You mention that it doesn't get called, so we're back at Grace trying to fulfill unregistered exports.
(I'm not too surprised that it's not called: this is a bit of a pattern for optional registrations.)

Without debugging, the behavior I would expect is:

• IEnumerable is always injectable, even if the items are not registered (empty collection).
• If NamedService<IGrainDirectory> is registered (as singleton) that's straightforward.
• If it's not registered then it depends on option AutoRegisterUnknown. When it's false, that should be the easy case where an empty collection is registered (but you mentionned it doesn't -> I need to debug the simple repro to figure it out).
• If it is, it can be sketchy. I had previous experiences (in Automatic registration of open generics can lead to failures #309) where Grace auto-register the concrete NamedService<> class, then thinks it can locate it, but then fails because IGrainDirectory cannot be located.
  I suspect something similar but it fails because it cannot provide the string parameter to NamedService<> ctor.
  Quite frankly, auto-registration can get wonky and I personally always disable it.

I will debug the simple repro when I have time to and let you know if I find something.

[stephenlautier]

UPDATE: @jods4 tested the same unit test with the current master and it works, alto with this unexpected behavior (at least to me) which i posted above #315 (comment)

[jods4]

Well, it looks exactly like #309.

I have created this small test case, without MS DI:

With AutoGenerateUnknowns = false it works exactly as expected: no registration exists and an empty list is produced.

With AutoGenerateUnknowns = true, the missing exporters logic decides to register NamedService<>, as it has a public ctor, without realizing it won't be able to fulfill any of its ctor dependencies.

This is a can of worms, so I'm hesitant to fix it myself -- and #309 was already failing for similar cases in 7.0.
I suppose the fix would be to verify if all parameters of said ctor could be imported -- or else could be auto-registered, too.
It's not simple, I'll sleep over it and we'll see.

The fact you use AllowInstanceAndFactoryToReturnNull = true might as well have interactions with this logic, I didn't test it.

If you can I would suggest staying away from both AutoGenerateUnknowns and AllowInstanceAndFactoryToReturnNull.

[jods4]

@stephenlautier In case you'd want to keep AutoGenerateUnknowns = true, Grace has a configuration to exclude certain types from auto-registering: ExcludeTypeFromAutoRegistration.
The test case below passes, even though it has auto-registration enabled:

Just to ensure there is no unintended regression here I ran the same test on Grace 7.2 and got the same result (Could not locate type String).

@ipjohnson It is unfortunate that out of the box, Grace fails with obscure errors when using some important libraries, such as MassTransit or Orleans.

I can think of three possible ways to improve the situation, all of which would be breaking changes:

1. Be more specific in ShouldCreateConcreteStrategy

This was my first idea but I don't quite like it.

Firstly, it's practically impossible to account for all the ways a ctor parameter can be injected. Adding conditions is going to be a trade-off between false positive (creating a registration that's impossible to fulfill) and false negative (not auto-registering something that could be).

Even if the change was narrow and targeted at this issue, say: don't use ctor with primitive or string types, there's a chance it breaks someone that has a fulfilled string parameter. It could come from a keyed registration ([ImportKey("environment")] string), the injection context, or maybe we're in a Delegate strategy and the string parameter is passed from delegate.

Second, from a DX perspective I like the current flow better. If you perform Locate<BadType>() and expect it to auto-register, I think it's good that we register it and attempt a resolution, then provide a detailed message to explain where the locate graph failed (e.g. in this bug report it was clear that the string parameter from ctor was the issue).
If we filter more accurately, suddenly this kind of Locate will fail because of a "missing export", which can be surprising if you expect auto-registration.

2. Don't auto-register collections

In the same spirit that TryLocate (GetService vs GetRequiredService in MS DI) would not auto-register a missing export, one can argue that IEnumerable should return an empty list rather than attempting registrations.

It's a weird use-case because auto-registration only works with concrete types, and locating an auto-registered IEnumerable<Concrete> would never have more than one activation anyway?!

The drawback of this is that if anyone depends on this, there's no workaround to the breaking change, beside explicit registration.

3. AutoGenerateUnknowns = false by default

This is my favorite.

The migration from Grace 7 for users that depend on AutoGenerateUnknowns is to turn it on explicitly. Nothing else changes.

For new users, I think it's a better default.

I think people expect a DI framework (in general) to require registration before usage. I would bet most users are unaware that they have auto-registration potentially going on behind the scenes.

And the problem with auto-registration is that it's wonky and unpredictable. It's easy to register things you don't want (and maybe don't notice: internal options from libraries for example), with inappropriate lifetimes.
At worst, without careful configuration it can break code, as demonstrated in MassTransit or Orleans.

What do you think?

[stephenlautier]

@jods4 Thanks for you suggestions

RE: AutoGenerateUnknowns
I thought it was the default from MS DI, that concrete types gets auto created, but I think its because I've been using Grace for several years now 😅
But yes, I agree if thats the default behavior generally, might be a better option to use that and default it to false, and I agree completely regarding unpredictable liftimes and such with auto registration.
Configuring that to false Orleans also works, sorry; I think when you asked me to test with it I had done it true instead 🙈

Our code makes use of the AutoGenerateUnknowns (mostly unintentionally), but we can go through those and fix them easily; so im happy with your suggestion. Thanks!!

[jods4]

@stephenlautier did you see my last comment above? If you depend on auto-registration you can also just exclude NamedService with ExcludeTypeFromAutoRegistration.

[stephenlautier]

@stephenlautier did you see my last comment above? If you depend on auto-registration you can also just exclude NamedService with ExcludeTypeFromAutoRegistration.

Yes, thank you i've seen it. That would make it easier to switch; but I agree with your statements above so changing from our end its also good thing.

Now im looking into another issue, not sure if its a Grace issue or not so far as im trying to narrow it down, but it is possible though.
Scoped services e.g. GraceServiceScope seems to be disposing singletons also and not just the scoped instances :'(

[jods4]

Let me know if you find a regression.
I had to tweak lifecycles a bit for Any key registrations, but I have not touched the disposal logic at all.
Nothing's impossible but it would be a surprising regression (at least from my PR, there were other commits made after 7.2 that I know nothing of).

[stephenlautier]

@jods just in case i created another issue which is #316 (comment) related to the issue i described above
from what i see its not a regression, but related with disposing of keyed services when they are scoped (but a basic example works, so probably an edge case of some sort)