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

Implemented IServiceProviderIsService #33

Merged
merged 11 commits into from
Nov 6, 2023
Merged

Implemented IServiceProviderIsService #33

merged 11 commits into from
Nov 6, 2023

Conversation

CoryCharlton
Copy link
Member

@CoryCharlton CoryCharlton commented Oct 10, 2023

Description

  • Added GetServiceDescriptors(type) to retrieve registered ServiceDescriptor
  • Replace GetService(type) with call to GetServiceDescriptors(type) in GetParameters(Type implementationType)
  • Bail early in GetParameters(Type implementationType) as soon as we find out that we cannot support the current constructor

Motivation and Context

How Has This Been Tested?

  • Unit tests
  • On device

Screenshots

Types of changes

  • Improvement (non-breaking change that improves a feature, code or algorithm)
  • Bug fix (non-breaking change which fixes an issue with code or algorithm)
  • New feature (non-breaking change which adds functionality to code)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Config and build (change in the configuration and build system, has no impact on code or features)
  • Dependencies (update dependencies and changes associated, has no impact on code or features)
  • Unit Tests (add new Unit Test(s) or improved existing one(s), has no impact on code or features)
  • Documentation (changes or updates in the documentation, has no impact on code or features)

Checklist:

  • My code follows the code style of this project (only if there are changes in source code).
  • My changes require an update to the documentation (there are changes that require the docs website to be updated).
  • I have updated the documentation accordingly (the changes require an update on the docs in this repo).
  • I have read the CONTRIBUTING document.
  • I have tested everything locally and all new and existing tests passed (only if there are changes in source code).
  • I have added new tests to cover my changes.

@nfbot nfbot added Type: bug Something isn't working Type: enhancement New feature or request labels Oct 10, 2023
@CZEMacLeod
Copy link

CZEMacLeod commented Oct 11, 2023

The test to see if the service is available from the service provider is something that is wrapped up in the IServiceProviderIsService.IsService(Type) method.
It would probably be better to try and get the IServiceProviderIsService type from the IServiceProvider, and, if available, then use that to check if the type required is available. If the interface is not available on the IServiceProvider (which it would be for the built-in instance), then you can fall back to the existing mechanism of trying to instanciate the object.
This would provide the same functionality, as the existing code in this PR, but map more closely with MEDI and maybe other code out there could also use this call if needed.

Here is a blog post describing this same issue and how it is resolved in net6 - https://andrewlock.net/exploring-dotnet-6-part-10-new-dependency-injection-features-in-dotnet-6/#detecting-if-a-service-is-registered-in-the-di-container and the original issue in dotnet dotnet/runtime#53919 and the PR that solved that (I totally understand that, aside from the interface, your solution will probably be quite different) - https://github.com/dotnet/runtime/pull/54047/files

@CoryCharlton CoryCharlton changed the title Added GetServiceDescriptors() so a throwaway object is not created for transient services during constructor resolution Implemented IServiceProviderIsService so a throwaway instance is not created for transient services during constructor resolution Oct 11, 2023
@CoryCharlton
Copy link
Member Author

@CZEMacLeod @josesimoes I added and implemented IServiceProviderIsService. This solution ends up being more efficient than the previous GetServiceDescriptors since we bail as soon as we know a service is registered rather than iterating over the entire collection every time.

I removed my GetServiceDescriptors implementation as it was only being used where IsService is now and I didn't see any immediate benefits on refactoring any of the other code to use it.

@CoryCharlton
Copy link
Member Author

Not directly related to this PR but should AggregateException be moved to the core library?

@josesimoes
Copy link
Member

Not directly related to this PR but should AggregateException be moved to the core library?

If we're going to have it, yes. But that one depends heavily on generics which we still don't have.
It would require a specific implementation to overcome that.
This to say what we would be deviating from the full .NET anyways.

Having said that, let me ask you this: is this for a single specific location in the code? Or do you need this at several methods?
The answer to this will point us to the answer. Probably a "dedicated" exception for that case is enough.

@CZEMacLeod
Copy link

While looking at the code for this, I think the ServiceProviderEngine.GetParameters function actually has a mistake in it (at least it doesn't perform the same logic as MEDI) and I think has the potential to misbehave.
I'll raise an issue for it separately, but wanted to flag it here too, as this reorg and potential breaking makes it tricky to do a PR on just now.
I can't easily create and test an equivalent nano project just now, but I'm pretty sure the following Net 7 code would break on nano (notwithstanding the use of generics).

The current code checks if there are duplicates of the longest constructor length - in the example below there are 3x 2 parameter constructors.
In MEDI, it checks if there are multiple resolvable constructors of the same (max) length to throw the ambiguous error. (Comment out the public RegisteredService3(RegisteredService1 service1, RegisteredService2 service2) constructor).

using Microsoft.Extensions.DependencyInjection;

internal class Program
{
    private static void Main(string[] args)
    {
        var services = new ServiceCollection();

        services.AddSingleton<RegisteredService1>();
        services.AddSingleton<RegisteredService2>();

        services.AddSingleton<RegisteredService3>();

        var serviceProvider = services.BuildServiceProvider();

        var test = serviceProvider.GetRequiredService<RegisteredService3>();

        Console.WriteLine($"HasService1: {test.HasService1}");
        Console.WriteLine($"HasService2: {test.HasService2}");
        Console.WriteLine($"HasUnregistered: {test.HasUnregistered}");
    }
}

internal class RegisteredService1
{
    public RegisteredService1()
    {
    }
}

internal class RegisteredService2
{
    public RegisteredService2()
    {
    }
}

internal class RegisteredService3
{
    private readonly RegisteredService1? service1;
    private readonly UnRegisteredService? unRegistered;
    private readonly RegisteredService2? service2;

    public RegisteredService3(RegisteredService1 service1) => this.service1 = service1;

    public RegisteredService3(RegisteredService2 service2) => this.service2 = service2;

    public RegisteredService3(RegisteredService1 service1, UnRegisteredService unRegistered) : this(service1) => this.unRegistered = unRegistered;

    public RegisteredService3(RegisteredService2 service2, UnRegisteredService unRegistered) : this(service2) => this.unRegistered = unRegistered;

    public RegisteredService3(RegisteredService1 service1, RegisteredService2 service2)
    {
        this.service1 = service1;
        this.service2 = service2;
    }

    public bool HasService1 => service1 != null;
    public bool HasService2 => service2 != null;
    public bool HasUnregistered => unRegistered != null;
}

internal class UnRegisteredService
{
    public UnRegisteredService()
    {
    }
}

@josesimoes
Copy link
Member

@CZEMacLeod let's add some unit test to cover this please! 😉

@CoryCharlton
Copy link
Member Author

While looking at the code for this, I think the ServiceProviderEngine.GetParameters function actually has a mistake in it (at least it doesn't perform the same logic as MEDI) and I think has the potential to misbehave.

Thanks for bringing this up. I saw this and wondered if this was an issue or intentional. My assumption was that something like this should work:

public class Service1 : IService1
{
    private readonly ILogger _logger;

    public Service1(ILoggerFactory loggerFactory): this(loggerFactory.CreateLogger(nameof(Service1)))
    {
        
    }

    public Service1(ILogger logger = null)
    {
        _logger = logger;
    }
}

My expectation was that the first resolvable constructor with the highest number of parameters would be used. It sounds like that expectation isn't the case in MEDI either but should it be?

If you open an issue I'll add my two cents there.

@CZEMacLeod
Copy link

@CoryCharlton In your example MEDI would throw an error if both ILogger and ILoggerFactory was registered. In normal dotnet, this wouldn't be the case, as it would be ILogger<Service1> or ILogger<IService1> and a basic ILogger would not be registered but there is an example in the MEDI documentation that covers this scenario and that would throw as it can't work out which to use.
There is an attribute ActivatorUtilitiesConstructorAttribute which can be used to mark which constructor to use in cases like this, although I haven't checked the details of exactly how it is implemented.
Another thing I was confused by when looking at creating a PR for this, was that it seems to resolve all intrinsic/primative types to their default value - something that MEDI does not do at all. I am not sure the reason for this behaviour as it is very counter intuitive. If I wanted this then I would .AddSingleton(string.Empty), or .AddSingleton<string>(null) - although I think that actually throws an error as you cannot register a null object for a service.
Let me get my thoughts on this together and raise an actual issue. I don't know if a discussion on this to decide how close this should work to the MEDI implementation and what deviations and justifications for those differences should be, would be useful, either here or on discord. In any case, I think they need to be documented clearly so that anyone who is looking at the main net documentation and comes here knows the caveats and does not get any surprises.

@josesimoes
Copy link
Member

Wow! I'm delighted to see the level of detail and how technical this conversation is!
Humbled and thankfully for the time and perfection you're putting into nanoFramework and these tiny details of the implementation.
And the concern on making developers life easier and getting this as close as possible from the full framework.
Really profesional!! On behalf of the project and the community, thank you for helping on taking our code base to a top notch quality level. 🤩💯🙂

@josesimoes
Copy link
Member

Any further developments on this?

@josesimoes josesimoes requested review from bytewizer and removed request for bytewizer October 31, 2023 10:19
@CoryCharlton
Copy link
Member Author

Any further developments on this?

I reviewed the thread and there was lots of discussion but I believe all pending action items for this PR have been resolved.

@CZEMacLeod
Copy link

@CoryCharlton @josesimoes Yeah - I think the changes/issues I have don't compete with this PR (and I was assuming that any changes would be based off this work).
I think there are some breaking changes here, and if the changes to check ambiguous constructors and default value intrinsics were added too, that would also potentially be a breaking change.
I do have the code to make those changes, but I need to finish the unit tests, as the activator and DI rules don't match the normal implementation just now.

@CoryCharlton
Copy link
Member Author

Speaking of breaking changes I realized I should probably update the major version with this PR so I just did so.

@josesimoes
Copy link
Member

I do have the code to make those changes, but I need to finish the unit tests, as the activator and DI rules don't match the normal implementation just now.

Great! Let's have that PR with more fixes/improvements.
If you're stuck with the Unit Tests just ping me and I'll help you with that.

Copy link
Member

@josesimoes josesimoes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK this LGTM!
Thank you again for this nice improvement/fix.

@josesimoes
Copy link
Member

Speaking of breaking changes I realized I should probably update the major version with this PR so I just did so.

This calls for a bump in version but minor, not major.
I've just changed it.

Copy link

sonarcloud bot commented Nov 2, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 6 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link

@microcompiler microcompiler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks fine to me. Just a reminder we will need to update the samples to support these changes.

@CoryCharlton
Copy link
Member Author

This looks fine to me. Just a reminder we will need to update the samples to support these changes.

I started this work here nanoframework/Samples#332

Copy link
Contributor

@bytewizer bytewizer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm good with it. Sorry wrong account.

@josesimoes josesimoes changed the title Implemented IServiceProviderIsService so a throwaway instance is not created for transient services during constructor resolution Implemented IServiceProviderIsService Nov 6, 2023
@josesimoes josesimoes merged commit a09fbd7 into nanoframework:main Nov 6, 2023
3 checks passed
@josesimoes
Copy link
Member

Merged. Thank you very much everyone on this so participated PR! 😄
Please let's follow up on all the conversations above and go for PRs on all those bits that can be further improved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: bug Something isn't working Type: enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Transient services are instantiated twice while creating a service that depends on them
6 participants