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

ServiceHandle support for Singletons #490

Open
buko opened this issue Feb 1, 2020 · 2 comments
Open

ServiceHandle support for Singletons #490

buko opened this issue Feb 1, 2020 · 2 comments

Comments

@buko
Copy link

buko commented Feb 1, 2020

There are many, many places in our code where we execute the following logic:

for (ServiceHandle<Foo> handle : findServices()) 
   try (handle) {
      handle.getService().foo();
   }

The idea here is we're going to find handles for all Foos, create each one, do something and then destroy the service. We don't really care what scope the Foo is in... we just want to make sure it gets properly created and destroyed.

The thing is we actually do kind of care about scope. The vast majority of our services are in the @singleton scope and the ServiceHandle#close will actually call SingletonContext#destroyOne on these services. The logic above will result in many Singleton services being destroyed and then unnecessarily recreated again.

I'd propose that the ServiceHandle#close method should not call context.destroyOne for objects in the Singleton scope. These services really should not be destroyed until the SingletonContext is destroyed.

In code terms the ServiceHandle#close method would be modified to check for a Singleton Scope and if it's a Singleton it would immediately return:

if (root.getScopeAnnotation().equals(Singleton.class))
  return;

This isn't an API change being requested it's a subtle change in behavior. What do others think?

@Cousjava
Copy link
Member

Cousjava commented Mar 4, 2020

This is an API change, as the Javadocs for ServiceHandle says on the class level

This service handle can be used to get a specific instance of a service,
 and can be used to destroy that service as well

and also on the close method

Will destroy this object and all PerLookup instances created of this service

This make it clear that a ServiceHandle can be used to destroy an instance, regardless of scope and there also tests that require this to be the case.

What non-singleton scopes do you use? @PerLookup looks like what you want to be used for all non-Singletons, as that is designed for what you have here, where you care creating and then destroying an instance.

@buko
Copy link
Author

buko commented Mar 17, 2020

I guess it is an API change due to Javadoc.

I just don't understand the point of destroying objects in the Singleton Scope. These objects will be destroyed when the Singleton scope is destroyed and they are specially designed to be reused.

@PerLookup looks like what you want to be used for all non-Singletons, as that is designed for what you have here, where you care creating and then destroying an instance.

My services are mostly in the Singleton scope. But when I access them through the ServiceHandle API I don't know for sure that the referenced Service will be in the Singleton scope. For this reason I always make sure that getService() is followed by a close(). The problem is that 99% of the time this leads to unnecessarily destroying a Singleton-scoped service. And the next time I look up the service I have to create it. So our system ends up taking longer than it should to start because we spend a bunch of time needlessly destroying and re-creating Singleton services.

I guess the question is what is ServiceHandle really trying to do. I had always thought of it more as a, well, handle. I would not expect that destroying the handle would destroy the underlying Service unless the creation of the handle created the underlying service. Is this the case? If I destroy a ServiceHandle to a Singleton service A that is being used by other services B and C, will B and C now be working with a destroyed Singleton that they injected?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants