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

[release/8.0-staging] Add support for enumerating keyed services with KeyedService.AnyKey #95853

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Dec 11, 2023

Backport of #95582 to release/8.0-staging

Note that there will be a follow-up PR that will also be backported; see #95582 (comment)

/cc @benjaminpetit

Customer Impact

Keyed services in DI are new in 8.0, and this addresses an issue when KeyService.AnyKey is used when quering via IServiceProvider.GetKeyedServices<IService>(KeyedService.AnyKey).

Previously, registering a keyed service with a null key would show up when calling GetKeyedServices(), now they won't be. When null is used at a key for a service, the service is not considered a keyed service.
 

Testing

Tests were added to the original PR.

Risk

See above; there is some pending follow-up work that should be done in the same versioning release.

Other

  • Added <GeneratePackageOnBuild>true</GeneratePackageOnBuild>
  • Added <ServicingVersion>1</ServicingVersion>

@ghost
Copy link

ghost commented Dec 11, 2023

Tagging subscribers to this area: @dotnet/area-extensions-dependencyinjection
See info in area-owners.md if you want to be subscribed.

Issue Details

Backport of #95582 to release/8.0-staging

/cc @benjaminpetit

Customer Impact

Testing

Risk

IMPORTANT: If this backport is for a servicing release, please verify that:

  • The PR target branch is release/X.0-staging, not release/X.0.

  • If the change touches code that ships in a NuGet package, you have added the necessary package authoring and gotten it explicitly reviewed.

Author: github-actions[bot]
Assignees: -
Labels:

area-Extensions-DependencyInjection

Milestone: -

@stephentoub
Copy link
Member

@benjaminpetit, @steveharter, was mail sent to tactics about this one yet? Thanks.

Copy link
Member

@halter73 halter73 left a comment

Choose a reason for hiding this comment

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

I don't think we should merge this until we address the bug I describe in #95582 (comment) and add tests for GetKeyedService<IService>(KeyedService.AnyKey) and multiple AnyKey registrations.

@ghost ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Dec 28, 2023
@carlossanlop
Copy link
Member

carlossanlop commented Jan 9, 2024

The DependencyInjection.csproj is marked as IsPackable=true, meaning it is an OOB package. It needs package authoring changes to ensure it gets built.

Here's the documentation: https://github.com/dotnet/runtime/blob/release/8.0-staging/docs/project/library-servicing.md

@carlossanlop
Copy link
Member

Friendly reminder that Tuesday January 16th 4pm is the Code Complete deadline for the February Release. If the feedback gets addressed, please merge before that date and time to ensure this fix gets included in that Release. Otherwise, it'll have to wait until March.

@carlossanlop carlossanlop added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Jan 16, 2024
@ghost ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Jan 17, 2024
@steveharter steveharter added this to the 8.0.x milestone Jan 18, 2024
@steveharter steveharter self-assigned this Jan 18, 2024
@benjaminpetit
Copy link
Member

I think as pointed in #95582, it needs more work

@carlossanlop
Copy link
Member

Friendly reminder that Monday February 12th is the Code Complete deadline for the March Release. If the feedback gets addressed, please merge before that date to ensure this fix gets included in that Release. Otherwise, it'll have to wait until April.

@carlossanlop
Copy link
Member

Friendly reminder that Monday March 11th is the Code Complete date for the April Release. If you want this change to be included in that release, please address the remaining blocking feedback, get a sign-off from an area owner, and send an email to Tactics requesting approval. @benjaminpetit @halter73

@stephentoub
Copy link
Member

@benjaminpetit, where are we with this?

@steveharter
Copy link
Member

@benjaminpetit should we close this PR for now?

@benjaminpetit
Copy link
Member

Yes, we first need to reopen and finish #97561 (I don't think we have consensus right now on the right approach)

@robertmclaws
Copy link

So is anything happening with this? I would really use it in several of my apps.

@carlossanlop
Copy link
Member

@benjaminpetit @halter73 friendly reminder that code complete for the August Release is July 15th. If you want this fix to be included in that release, please merge this PR before that date.

@carlossanlop
Copy link
Member

Friendly reminder that Monday July 15th is Code Complete day, that's the deadline to get this included in the August Release.

@halter73 halter73 self-requested a review July 12, 2024 19:58
@carlossanlop
Copy link
Member

@halter73 @benjaminpetit this has been open for 8 months, Let's close it and come back to it if we decide we want it fixed.

@robertmclaws
Copy link

Can we PLEASE fix this? I have certain scenarios for my apps that don't work properly because AnyKey does not work properly.

@jkotas jkotas deleted the backport/pr-95582-to-release/8.0-staging branch August 16, 2024 04:46
@github-actions github-actions bot locked and limited conversation to collaborators Sep 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Extensions-DependencyInjection NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants