Skip to content

Add support for auto-activated keyed singletons - #4222

Merged
geeknoid merged 1 commit into
mainfrom
geeknoid/keyed
Aug 5, 2023
Merged

Add support for auto-activated keyed singletons#4222
geeknoid merged 1 commit into
mainfrom
geeknoid/keyed

Conversation

@geeknoid

@geeknoid geeknoid commented Jul 27, 2023

Copy link
Copy Markdown
Member

null

Microsoft Reviewers: Open in CodeFlow

@ghost ghost assigned geeknoid Jul 27, 2023
@geeknoid

Copy link
Copy Markdown
Member Author

Tests will fail in this PR until dotnet/runtime#89447 is fixed.

@RussKie

RussKie commented Jul 30, 2023

Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@RussKie

RussKie commented Jul 30, 2023

Copy link
Copy Markdown
Contributor

We're blocked by dotnet/aspnetcore#49704.

@geeknoid
geeknoid force-pushed the geeknoid/keyed branch 2 times, most recently from ea12738 to 7500a1f Compare August 2, 2023 16:39
@RussKie

RussKie commented Aug 3, 2023

Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@RussKie

RussKie commented Aug 3, 2023

Copy link
Copy Markdown
Contributor

Microsoft.Extensions.DependencyInjection.AutoActivation.Tests are failing

@xakep139

xakep139 commented Aug 3, 2023

Copy link
Copy Markdown
Contributor

Microsoft.Extensions.DependencyInjection.AutoActivation.Tests are failing

Tests failed because of the same reason: System.InvalidOperationException : This service provider doesn't support keyed services.

@xakep139

xakep139 commented Aug 3, 2023

Copy link
Copy Markdown
Contributor

dotnet/aspnetcore#49704 was merged 14 hours ago, I will try to update the repo.

@xakep139

xakep139 commented Aug 3, 2023

Copy link
Copy Markdown
Contributor

It seems that the latest build failed: https://dev.azure.com/dnceng/internal/_build/results?buildId=2235253&view=logs&j=316d5c15-0c50-544e-8051-e6b14a1ab674&t=976ed21e-c736-59e1-f431-046bb8d7ebcd

Thus, there's no artifacts to consume on our side yet.

@RussKie

RussKie commented Aug 4, 2023

Copy link
Copy Markdown
Contributor

Hopefully #4240 brought the latest changes.

@xakep139

xakep139 commented Aug 4, 2023

Copy link
Copy Markdown
Contributor

@geeknoid can you please try to merge main into this branch and see whether it helps?

@geeknoid

geeknoid commented Aug 4, 2023

Copy link
Copy Markdown
Member Author

@geeknoid can you please try to merge main into this branch and see whether it helps?

Yep, the fundamental issue is fixed. Now I just have to get my own shitty code working :-)

@geeknoid
geeknoid force-pushed the geeknoid/keyed branch 3 times, most recently from ad309ad to 2c787ce Compare August 5, 2023 03:14
@geeknoid

geeknoid commented Aug 5, 2023

Copy link
Copy Markdown
Member Author

@xakep139 This PR is finally ready to go...

/// <param name="serviceKey">The <see cref="ServiceDescriptor.ServiceKey"/> of the service.</param>
/// <returns>A reference to this instance after the operation has completed.</returns>
[Experimental(diagnosticId: Experiments.AutoActivation, UrlFormat = Experiments.UrlFormat)]
public static IServiceCollection ActivateKeyed<TService>(

@xakep139 xakep139 Aug 5, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we rename it to ActivateKeyedSingleton? All DI-related extension method specify desired lifetime it their name (e.g. AddScoped(), AddTransient(), etc.)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Alternatively, if we don't care about lifetime, we should remove singleton from XML docs

}

/// <summary>
/// Adds an auto-activated singleton service of the type specified in TService with an implementation

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Adds an auto-activated singleton service of the type specified in TService with an implementation
/// Adds an auto-activated singleton service of the type specified in <c>TService</c> with an implementation


/// <summary>
/// Adds an auto-activated singleton service of the type specified in TService with an implementation
/// type specified in TImplementation using the factory specified in implementationFactory

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// type specified in TImplementation using the factory specified in implementationFactory
/// type specified in <c>TImplementation</c> using the factory specified in <paramref name="implementationFactory"/>

}

/// <summary>
/// Adds an auto-activated singleton service of the type specified in serviceType with a factory

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There's no serviceType in this overload

}

/// <summary>
/// Adds an auto-activated singleton service of the type specified in TService with an implementation

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Adds an auto-activated singleton service of the type specified in TService with an implementation
/// Adds an auto-activated keyed singleton service of the type specified in TService with an implementation

.AddActivatedKeyedSingleton<IFakeService, FakeService>(serviceKey))
.StartAsync();

var service = host.Services.GetKeyedService<IFakeService>(serviceKey);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What if we also check that instanceCount.Counter equals to 1 before we get the service from the provider?

.AddActivatedKeyedSingleton(typeof(IFakeService), serviceKey, typeof(FakeService))
.AddActivatedKeyedSingleton<IFactoryService, FactoryService>(serviceKey, (sp, sk) =>
{
return new FactoryService(sp.GetKeyedService<IFakeService>(sk)!, sp.GetRequiredService<IFactoryServiceCounter>()!);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
return new FactoryService(sp.GetKeyedService<IFakeService>(sk)!, sp.GetRequiredService<IFactoryServiceCounter>()!);
return new FactoryService(sp.GetKeyedService<IFakeService>(sk)!, sp.GetRequiredService<IFactoryServiceCounter>());

}

[Fact]
public async Task ShouldAddAndActivateOnlyOnce_WhenHasChildAsync_Keyed()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
public async Task ShouldAddAndActivateOnlyOnce_WhenHasChildAsync_Keyed()
public async Task ShouldAddAndActivateOnlyOnce_WhenHasChild_Keyed()

}

[Fact]
public async Task ShouldResolveComponentsAutomaticallyAsync_Keyed()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
public async Task ShouldResolveComponentsAutomaticallyAsync_Keyed()
public async Task ShouldResolveComponentsAutomatically_Keyed()

}

[Fact]
public async Task CanActivateEnumerableAsync_Keyed()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
public async Task CanActivateEnumerableAsync_Keyed()
public async Task CanActivateEnumerable_Keyed()

Assert.Equal(0, anotherFakeServiceCount.Counter);
}

// ------------------------------------------------------------------------------

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Remove that line?

@geeknoid
geeknoid merged commit 0316e5e into main Aug 5, 2023
@geeknoid
geeknoid deleted the geeknoid/keyed branch August 5, 2023 15:15
@ghost ghost added this to the 8.0 RC1 milestone Aug 5, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Sep 4, 2023
@github-actions github-actions Bot added the area-ai Microsoft.Extensions.AI libraries label Mar 7, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-ai Microsoft.Extensions.AI libraries

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants