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

[API Proposal]: Add ServiceCollection.MakeReadOnly() #66126

Closed
halter73 opened this issue Mar 3, 2022 · 2 comments · Fixed by #68051
Closed

[API Proposal]: Add ServiceCollection.MakeReadOnly() #66126

halter73 opened this issue Mar 3, 2022 · 2 comments · Fixed by #68051
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-Extensions-DependencyInjection
Milestone

Comments

@halter73
Copy link
Member

halter73 commented Mar 3, 2022

Background and motivation

The new HostApplicationBuilder in Microsoft.Extensions.Hosting and the MAUI host both implement custom IServiceCollection that change to read-only after the host is built. This means that ICollection<ServiceDescriptor>.IsReadOnly becomes true and any attempts to mutate the collection result in an InvalidOperationException. This makes it clear to developers that any attempt to modify services at this point is futile.

It would be nice if these hosts could use the default ServiceCollection to take advantage of potential perf improvements like those described in #44728.

API Proposal

namespace Microsoft.Extensions.DependencyInjection
{
    public class ServiceCollection
    {
+      public void MakeReadOnly();
    }
}

API Usage

public ServiceCollection ServiceCollection { get; } = new();

IHost Build()
{
    // BuildServiceProvider() is an extension method on IServiceProvider so it cannot
    // make the ServiceCollection read-only itself. It could after this change if it attempted to cast,
    // but that would be a breaking change and is not what I'm proposing.
    IServiceProvidier serviceProvider = ServiceCollection.BuildServiceProvider();
    ServiceCollection.MakeReadOnly();
    return serviceProvider.GetRequiredService<IHost>();
}

Alternative Designs

namespace Microsoft.Extensions.DependencyInjection
{
    public class ServiceCollection
    {
-      public bool IsReadOnly { get; }
+      public bool IsReadOnly { get; set; }
    }
}

I considered adding a setter to IsReadOnly, but I don't think it should be possible to transition a ServiceCollection from a read-only to a mutable state.

Risks

It adds additional API to ServiceCollection that shouldn't be called by the majority of developers interacting with the type. It's only really useful to library authors who build the ServiceCollection.

Context: #65109 (comment)

@eerhardt

@halter73 halter73 added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Mar 3, 2022
@dotnet-issue-labeler dotnet-issue-labeler bot added area-Extensions-DependencyInjection untriaged New issue has not been triaged by the area owner labels Mar 3, 2022
@ghost
Copy link

ghost commented Mar 3, 2022

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

Issue Details

Background and motivation

The new HostApplicationBuilder in Microsoft.Extensions.Hosting and the MAUI host both implement custom IServiceCollection that change to read-only after the host is built. This means that ICollection<ServiceDescriptor>.IsReadOnly becomes true and any attempts to mutate the collection result in an InvalidOperationException. This makes it clear to developers that any attempt to modify services at this point is futile.

It would be nice if these hosts could use the default ServiceCollection to take advantage of potential perf improvements like those described in #44728.

API Proposal

namespace Microsoft.Extensions.DependencyInjection
{
    public class ServiceCollection
    {
+      public void MakeReadOnly();
    }
}

API Usage

public ServiceCollection ServiceCollection { get; } = new();

IHost Build()
{
    // BuildServiceProvider() is an extension method on IServiceProvider so it cannot
    // make the ServiceCollection read-only itself. It could after this change if it attempted to cast,
    // but that would be a breaking change and is not what I'm proposing.
    IServiceProvidier serviceProvider = ServiceCollection.BuildServiceProvider();
    ServiceCollection.MakeReadOnly();
    return serviceProvider.GetRequiredService<IHost>();
}

Alternative Designs

namespace Microsoft.Extensions.DependencyInjection
{
    public class ServiceCollection
    {
-      public bool IsReadOnly { get; }
+      public bool IsReadOnly { get; set; }
    }
}

I considered adding a setter to IsReadOnly, but I don't think it should be possible to transition a ServiceCollection from a read-only to a mutable state.

Risks

It adds additional API to ServiceCollection that shouldn't be called by the majority of developers interacting with the type. It's only really useful to library authors who build the ServiceCollection.

Author: halter73
Assignees: -
Labels:

api-suggestion, untriaged, area-Extensions-DependencyInjection

Milestone: -

@eerhardt eerhardt added this to the 7.0.0 milestone Mar 3, 2022
@eerhardt eerhardt added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation untriaged New issue has not been triaged by the area owner labels Mar 3, 2022
@terrajobst
Copy link
Member

  • Looks good as proposed
namespace Microsoft.Extensions.DependencyInjection;

public partial class ServiceCollection
{
    public void MakeReadOnly();
}

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Apr 14, 2022
@eerhardt eerhardt added the help wanted [up-for-grabs] Good issue for external contributors label Apr 14, 2022
@eerhardt eerhardt self-assigned this Apr 14, 2022
@eerhardt eerhardt removed the help wanted [up-for-grabs] Good issue for external contributors label Apr 14, 2022
eerhardt added a commit to eerhardt/runtime that referenced this issue Apr 14, 2022
With new hosting API patterns, it is useful to be able to mark a ServiceCollection
as read-only and disallow for any more modifications to it.

Fix dotnet#66126
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Apr 14, 2022
eerhardt added a commit that referenced this issue Apr 15, 2022
With new hosting API patterns, it is useful to be able to mark a ServiceCollection
as read-only and disallow any more modifications to it.

Fix #66126
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Apr 15, 2022
@ghost ghost locked as resolved and limited conversation to collaborators May 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-Extensions-DependencyInjection
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants