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

Rename RegisterSingleton<T>(T) to RegisterInstance #469

Closed
mikechamberlain opened this Issue Oct 11, 2017 · 3 comments

Comments

2 participants
@mikechamberlain

mikechamberlain commented Oct 11, 2017

Background

Reading the source, it seems that this overload:

container.RegisterSingleton<T>(Func<T> instanceCreator);

is equivalent to the following method when called with Lifestyle.Singleton:

container.Register<T>(Func<T> instanceCreator, Lifestyle lifestyle);

An API that offers two different ways of accomplishing the same thing isn't ideal, and wouldn't be a huge problem until we consider this other overload, with different semantics. This one registers a pre-created instance outside of the control of the container:

container.RegisterSingleton<TService>(TService instance);

The docs say:

NOTE: Do note that instances supplied by this method NEVER get disposed by the container, since the instance is assumed to outlive this container instance. If disposing is required, use the overload that accepts a Func< TResult> delegate.

Registering a pre-created instance is of course a valid requirement, but is relatively rare.

Registering a container-controlled singleton is a very common requirement, but we already have a way of doing this with the Register method.

Motivation

The reason I raise this concern is because I'm currently migrating a codebase from Unity to Simple Injector, and I see their equivalent method RegisterInstance used unnecessarily (and incorrectly) in many places, for instance:

unityContainer.RegisterInstance<IMyService>(new MyService(), 
    new ContainerControlledLifetimeManager());

where IMyService may implement IDisposable, so ends up leaking resources. Instead, in almost every case I found, the following Unity invocation would be more appropriate:

unityContainer.RegisterType<IMyService, MyService>(new ContainerControlledLifetimeManager());

I will be fixing all these registrations during the migration process, but as we have a large team, I am sure that old habits will die hard, and SI's RegisterSingleton overload will be abused similarly.

Discussion

I'd would like to discuss container.RegisterSingleton<T>(Func<T> instanceCreator).

  • What is its motivation?
  • Do we need two APIs that do the same thing?
    • If so, why not also have methods for RegisterTransient, RegisterScoped etc?
    • If not, should this overload be rethought/renamed/deprecated/removed?
  • Does this overload provide a trap for less-knowledgeable devs to fall into?
  • Is it confusing or surprising to have two overloads of the same method with different semantics and use cases? While the differences are documented, their behaviors are not immediately obvious just by looking at the API.
  • If removed, could the remaining overloads be renamed to reflect the remaining use-case they address, something like RegisterPreCreated, RegisterSingletonInstance, or RegisterContainerUncontrolled?

Overall, does it satisfy these two design principles?

@dotnetjunkie

This comment has been minimized.

Collaborator

dotnetjunkie commented Oct 11, 2017

You raise fair points. Let's first establish that creating an intuitive API is hard, really hard. Especially when we consider that what one may find intuitive, can be confusing for others. A good example of this is when developers migrate from some other container to Simple Injector. They find Simple Injector's behavior unintuitive, since they are simply used to the work of other containers.

We actually did try to get rid of the redundant RegisterSingleton methods when we started working on v3, since we wanted to minimize the API and present a more uniform way of making registrations. But when we put the first beta out, we noticed that this breaking change was too big for most of the users. So we had to get it back in. Plans are still get remove them in the future, but as a library maintainer we will always have to balance between keeping existing users happy and productive, and improving the library over time.

Is it confusing or surprising to have two overloads of the same method with different semantics and use cases?

It depends on how you look at it. It might make more sense if you take the following rule into consideration:

he who creates/owns a resource, should dispose it

This is a very basic design principle that holds, even outside the context of Simple Injector, or even outside the context of DI.

In this sense, Simple Injector assumes the instances created by factories to be created (and thus owned) by itself, while it assumes an instance supplied to Simple Injector 'externally owned'.

Such rule however is very hard to express succinctly in any API. Removing RegisterSingleton would not help much, since it would neither be intuitively clear when someone calls Register<T>(Func<T>, Lifestyle).

If removed, could the remaining overloads be renamed to reflect the remaining use-case they address, something like RegisterPreCreated, RegisterSingletonInstance, or RegisterContainerUncontrolled?

There is a balance between succinct naming and intuitively. Although long names might be more intuitive to a first-time user, it will annoy long-time users, and will get in the way when you have big DI configurations. The choice has been made in the past to get rid of RegisterSingleton in the future, and that solution is preferred over having a bunch of other RegisterXXX methods.

Just like we tried getting rid of RegisterSingleton, we did try to introduce RegisterInstance, which is actually a sensible name, but has the downside of introducing two names for the same: instance and singleton. But even this change was a too big step to take with the introduction to v3. But still, renaming that overload to RegisterInstance is far less painful than removing RegisterSingleton<T>(Func<T>) altogether.

So steps will be taken, but as some evil men once said: "the process of transformation, even if it brings revolutionary change, is likely to be a long one".

@mikechamberlain

This comment has been minimized.

mikechamberlain commented Oct 11, 2017

Thanks for your thoughtful response, and for illuminating me on the history of this method.

If the long term plan is to remove RegisterSingleton, then couldn't it (or at least the overload in question) be marked [Obsolete] right now? Would this be considered a breaking change?

@dotnetjunkie

This comment has been minimized.

Collaborator

dotnetjunkie commented Oct 11, 2017

When it comes to removing 'things' the strategy typically is:

  • First mark as Obsolete(false) in a minor release, e.g. v4.2.
  • Next mark as Obsolete(true) in the next major release, e.g. v5.0.
  • Mark as Non-browsable so that it is hidden in IntelliSense.
  • Remove completely in a minor release after that, e.g v5.2.

Marking something Obsolete, even Obsolete(false) must be considered a breaking change, but that has never prevented us from doing this in case we feel the benefits outweighs the trouble. In other cases we might do the Obsolete(false) on a major release, Obsolete(true) on the following major and remove it completely in major+2, but I don't think we actually did that with Simple Injector. But once your user base grows, you have obviously to be more careful.

@dotnetjunkie dotnetjunkie added this to the v4.1 milestone Mar 2, 2018

@dotnetjunkie dotnetjunkie changed the title from Do we really need container.RegisterSingleton<T>(Func<T> instanceCreator)? to Rename RegisterSingleton<T>(T) to RegisterInstance Mar 2, 2018

dotnetjunkie added a commit that referenced this issue Mar 3, 2018

RegisterSingleton<TService>(TService) and RegisterSingleton(Type, obj…
…ect) overloads replaced with RegisterInstance methods and made old methods obsolete. Fixes #469

dotnetjunkie added a commit to simpleinjector/Documentation that referenced this issue Mar 23, 2018

Updated documentation for new RegisterInstance<T>(T) and RegisterInst…
…ance(Type, object) methods that replace their RegisterSingleton counter-parts. simpleinjector/SimpleInjector#469
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment