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

Does transient instances support disposing? #14

Open
jinyus opened this issue Jan 6, 2024 · 9 comments
Open

Does transient instances support disposing? #14

jinyus opened this issue Jan 6, 2024 · 9 comments

Comments

@jinyus
Copy link
Contributor

jinyus commented Jan 6, 2024

Correct me if I'm wrong... I don't think this is supported but this snippet in your docs implies that it is.

final builder = IocContainerBuilder()
    ..add((container) => DatabaseConnection('my-connection-string'))
    ..add<UserRepository>(
      (container) => UserRepository(container<DatabaseConnection>()),
      dispose: (userRepository) => userRepository.dispose(),
    )

Based on the dispose extension, only scoped singletons are disposed. Therefore the dispose function in the above example will never be called.

Future<void> dispose() async {
assert(isScoped, 'Only dispose scoped containers');
for (final type in singletons.keys) {
//Note: we don't need to check if the service is a singleton because
//singleton service definitions never have dispose
final serviceDefinition = serviceDefinitionsByType[type]!;
//We can't do a null check here because if a Dart issue
serviceDefinition._dispose.call(singletons[type]);
await serviceDefinition._disposeAsync(singletons[type]);
}
singletons.clear();
}

@MelbourneDeveloper
Copy link
Owner

Hi @jinyus

Good point!

I will fix the code example. Thanks for pointing it out

A PR to fix the doco would be welcome

@jinyus
Copy link
Contributor Author

jinyus commented Jan 7, 2024

I was looking through the code while creating the PR and realized that there is nothing wrong with the docs because the services are treated as singletons when scoped; so the dispose method will be called.

There is a conundrum though, because factory singletons cannot have a dispose method:

}) : assert(
!isSingleton || dispose == null,
'Singleton factories cannot have a dispose method',
),

So there is a memory leak where singletons created in a scope will never be disposed of. Let me know if you're ok with allowing singletons to have a dispose method and I will submit the PR.

@MelbourneDeveloper
Copy link
Owner

Sorry, yes, stored services do need dispose.

It's not a memory leak. That's by design. If you create a scope with disposable services, you need to dispose of them when you're finished.

@jinyus
Copy link
Contributor Author

jinyus commented Jan 7, 2024

re: memory leak
I mean that factory singletons are never disposed in a scope because you cannot supply a dispose method when registering them. My PR fixes this.

eg:

final builder = IocContainerBuilder()
    ..addSingletonService(AuthenticationService())

final container = builder.toContainer();

final scope = container.scoped();

final authService = scope<AuthenticationService>();
print(authService.login('user', 'password'));

await scope.dispose(); // AuthenticationService is not disposed here

@MelbourneDeveloper
Copy link
Owner

MelbourneDeveloper commented Jan 7, 2024 via email

@jinyus
Copy link
Contributor Author

jinyus commented Jan 7, 2024

Here is the code for addSingletonService:

void addSingletonService<T>(T service) => addServiceDefinition(
ServiceDefinition<T>(
(container) => service,
isSingleton: true,
),
);

It doesn't allow you to supply a dispose method. So my code above would result in a memleak. Unless I'm mistaken.

@MelbourneDeveloper
Copy link
Owner

You can see here that the scoped method copies the whole service definition, which includes the dispose method

Screenshot_20240107-121824.png

@jinyus
Copy link
Contributor Author

jinyus commented Jan 7, 2024

Yes, but currently there is no way to supply a dispose method for Singletons, only transients allow it.

eg:

  ///Add a singleton service to the container.
  void addSingletonService<T>(T service) => addServiceDefinition(
        ServiceDefinition<T>(
          (container) => service,
          isSingleton: true,
        ),
      );

  ///1️⃣ Add a singleton factory to the container. The container
  ///will only call this once throughout the lifespan of the container
  void addSingleton<T>(
    T Function(
      IocContainer container,
    ) factory,
  ) =>
      addServiceDefinition<T>(
        ServiceDefinition<T>(
          (container) => factory(container),
          isSingleton: true,
        ),
      );

addSingleton and addSingletonService doesn't allow you to supply a dispose method.

@MelbourneDeveloper
Copy link
Owner

@jinyus ah yes! I see your point. I've have a look at this and see if there are any issues

Thanks 🙏🏼

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