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

Add a (better) way to notify services of container disposal #4

Open
Tracked by #8
freshgum-bubbles opened this issue Jun 20, 2023 · 3 comments · May be fixed by #80
Open
Tracked by #8

Add a (better) way to notify services of container disposal #4

freshgum-bubbles opened this issue Jun 20, 2023 · 3 comments · May be fixed by #80
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@freshgum-bubbles
Copy link
Owner

freshgum-bubbles commented Jun 20, 2023

Currently, containers are just dropped from the service with no warning:

public reset(options: { strategy: 'resetValue' | 'resetServices' } = { strategy: 'resetValue' }): this {
this.throwIfDisposed();
switch (options.strategy) {
case 'resetValue':
this.metadataMap.forEach(service => this.disposeServiceInstance(service));
break;
case 'resetServices':
this.metadataMap.forEach(service => this.disposeServiceInstance(service));
this.metadataMap.clear();
this.multiServiceIds.clear();
break;
default:
throw new Error('Received invalid reset strategy.');
}
return this;
}

This seems quite strange. Services would surely want some form of notification
as to when their container is disposed of.

While a seemingly simple concept, the notion of how individual services are meant
to respond to such a notification is ambiguous. For instance, in the case of a database
service, is it meant to close down its database connection(s) when the container is disposed?
Furthermore, in such a case, should we provide a way for the caller of .dispose or .reset to
know when all such asynchronous events have been concluded?

One further challenge is how you would provide an API for such a notification.
It would be awkward to make services conform to a standard interface for notification,
and any runtime type-checking of notification handler methods could incur runtime errors.

This somewhat ties into typestack/typedi#230:

allow containers to dispose of themselves, destroying all locally registered service

Proposed APIs

Addition of @DisposeService decorator

@Service([])
export class DatabaseService {
  @DisposeService
  async dispose(): Promise<void> { }
}

@Service([DatabaseService])
export class RootService {
  constructor (private database: DatabaseService) { }

  @DisposeService([DatabaseService])
  async dispose (): Promise<void> { }
}

The decorator allows a list of dependencies to be passed.
The dependencies are guaranteed not to be disposed before
the returned dispose promise is settled.

For example, in the case of RootService, it may want to
perform further operations on the database (flushing caches, etc.)
before the database is disposed of (which would close the connection).
In this case, it can declare that it requires DatabaseService to dispose of
the application correctly. As such, it can return a Promise that, until settled,
will have an undisposed reference to DatabaseService available.

Use HostContainer

This is already a no-go, but for posterity...

Using this API would mean that tests would have to pass in a container instance for potentially nothing.
Also, we'd have to expose an addDisposeListener or the likes onto the class, which feels awkward.


Definitely needs further investigation.

@freshgum-bubbles
Copy link
Owner Author

Oh, interesting.

I was wizzing about yesterday and must have missed this in ContainerInstance:

private disposeServiceInstance(serviceMetadata: ServiceMetadata, force = false) {
this.throwIfDisposed();
/** We reset value only if we can re-create it (aka type or factory exists). */
const shouldResetValue = force || !!serviceMetadata.type || !!serviceMetadata.factory;
if (shouldResetValue) {
/** If we wound a function named destroy we call it without any params. */
if (typeof (serviceMetadata?.value as Record<string, unknown>)['dispose'] === 'function') {
try {
(serviceMetadata.value as { dispose: CallableFunction }).dispose();
} catch (error) {
/** We simply ignore the errors from the destroy function. */
}
}
serviceMetadata.value = EMPTY_VALUE;
}
}

This seems like a major hack. The service might not be expecting the DI container
to be performing such operations in a brittle manner.
As there isn't a decided, explicit way to notify services of container disposal yet,
I'll be removing this behaviour in the next revision.

@freshgum-bubbles freshgum-bubbles added the enhancement New feature or request label Jun 25, 2023
@freshgum-bubbles freshgum-bubbles changed the title Add a way to notify services of container disposal Add a (better) way to notify services of container disposal Jun 25, 2023
@freshgum-bubbles freshgum-bubbles removed this from the Version 1 milestone Jul 4, 2023
@freshgum-bubbles
Copy link
Owner Author

One possible implementation of this could be as follows:

import { Service, OnDispose } from '@typed-inject/injector';

@Service([OnDispose()])
export class MyService {
  constructor (private onDispose: OnDispose) { }

  bootstrap () {
    this.onDispose(() => console.log('being disposed'));
  }
}

@freshgum-bubbles
Copy link
Owner Author

A much simpler implementation would be the usage of Symbol.dispose, part of the upcoming ECMAScript Explicit Resource Management proposal.

@freshgum-bubbles freshgum-bubbles linked a pull request Sep 9, 2023 that will close this issue
@freshgum-bubbles freshgum-bubbles self-assigned this Sep 9, 2023
@freshgum-bubbles freshgum-bubbles moved this to Todo in Version 1! Nov 27, 2023
@freshgum-bubbles freshgum-bubbles moved this from Todo to In Progress in Version 1! Nov 27, 2023
@freshgum-bubbles freshgum-bubbles added this to the Version 1 milestone Dec 16, 2023
@freshgum-bubbles freshgum-bubbles moved this from In progress to Todo in Version 1! Jun 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Todo
Development

Successfully merging a pull request may close this issue.

1 participant