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

Disposable #43490

Open
vsevolod19860507 opened this issue Sep 19, 2020 · 18 comments
Open

Disposable #43490

vsevolod19860507 opened this issue Sep 19, 2020 · 18 comments
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. type-enhancement A request for a change that isn't a bug

Comments

@vsevolod19860507
Copy link

Add an abstract class Disposable that contains a disposable method. To standardize work with classes that need to be released.

@lrhn lrhn transferred this issue from dart-lang/language Sep 20, 2020
@lrhn lrhn added area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. type-enhancement A request for a change that isn't a bug labels Sep 20, 2020
@lrhn
Copy link
Member

lrhn commented Sep 20, 2020

Dart currently uses either close (for sinks) or cancel (for sources) to mark the end of operation.
We can probably add a Disposable/Closable/Cancelable interface with one of those methods, and make the corresponding classes implement that interface, but it's unlikely that we can add methods to the existing interfaces.

@vsevolod19860507
Copy link
Author

vsevolod19860507 commented Sep 20, 2020

The idea is in the standard interface that would serve as a marker that the object that implements it needs to be released.
Just like in C# https://docs.microsoft.com/en-us/dotnet/api/system.idisposable?view=netcore-3.1

This will provide a convenient check for other code that will work with these classes.

  if (obj is Disposable) {
    obj.dispose();
  }

Instead of this:

try {
    (obj as dynamic).dispose();
  } on Object catch (_) {}

@lrhn
Copy link
Member

lrhn commented Sep 21, 2020

For dispose to actually work, it would have to return a Future because some disposable objects need asynchronous clean-up when you dispose them.
That means that the interface must have dispose return a Future, which again means that all operations which work with disposable objects must be asynchronous to handle that future.

That suggests that Dart object disposal is not homogeneous enough to be supported by a single interface. Having two interfaces (Disposable, AsyncDisposable) seems like it would just make things more complicated for everybody.

Dart has first class functions, so I'd prefer composition to inheritance. Instead of making objects Disposable, introduce a Disposer interface and let everybody provide their own way to be disposable.

That sound like it would be something like:

abstract class Disposer<T, R> {
  Disposer(T value, R dispose()) = _Disposer<T, R>;
  T get value;
  R dispose();
}
class _Disposer<T> implements Disposable<T> {
  final T value;
  final R Function() _dispose;
  _Disposable(this.value, this._dispose);
  R dispose() => _dispose();
}

Then, if you want to dispose something, you just wrap it in a Disposer. You can declare extensions for common classes:

extension SubscriptionDisposer<T> on StreamSubcription<T> {
  Disposer<StreamSubscription<T>, Future<void>> get disposer => Disposer(this, this.cancel);
}

@vsevolod19860507
Copy link
Author

vsevolod19860507 commented Sep 21, 2020

We can use generic Disposable.

void main() {
  final objA = D(A());
  final objB = D(B());
  final objC = D(C());

  objA.dispose();
  objB.dispose();
  objC.dispose();
}

abstract class Disposable<R> {
  R dispose();
}

class A implements Disposable<Future<int>> {
  @override
  Future<int> dispose() async {
    print('A disposed');
    return Future(() => 1);
  }
}

class B implements Disposable<void> {
  @override
  void dispose() {
    print('B disposed');
  }
}

class C {
  void dispose() {
    print('----');
  }
}

class D {
  final Object obj;

  D(this.obj);

  void dispose() {
    if (obj is Disposable) {
      (obj as Disposable).dispose();
    }
  }
}

@lrhn
Copy link
Member

lrhn commented Sep 22, 2020

A generic disposable means that generic disposal handling code cannot assume that the return value is a Future, nor that it isn't a Future. All dispose abstractions need to code for both, and at that point, we might as well define it as FutureOr<void> dispose().
(Which is generally discouraged as API design because it puts a burden on the receiver).

@vsevolod19860507
Copy link
Author

I'm understood, thank you.

@themisir
Copy link

By the way In C# we now can use async dispose method as well using IAsyncDisposable. So we can do same thing in dart too by providing multiple Disposable interfaces for both sync and async dispose operations.

abstract class Disposable {
  void dispose();
}

abstract class AsyncDisposable {
  Future<void> disposeAsync();
}

Also, I would rather have non-async Disposable interface rather than having to unsafely disposing dynamic or dealing with 3rd party disposable interfaces.

@Jonas-Sander
Copy link

@lrhn To be honest your solution looks also kind of complicated/complex for me at least😄

What about FutureOr<void> dispose()?

@lrhn
Copy link
Member

lrhn commented Jan 15, 2021

If the dispose can return either a future or not, then you always need to check, and be ready for it to be a future (or you can always await the value). Then it's basically safer to just make the return type Future<void> and just wait for it every time.
Which makes everything asynchronous (but no more or less asynchronous than FutureOr<void> because that could be a future too). Generic code would still need to be asynchronous in order to be able to handle futures, even when no futures actually occur.

And it's bad API design to return a FutureOr. Be async, or be sync, there is no stable middle ground.

@alexeyinkin
Copy link

Most of the time we need to just kick-start the disposal and assume the resources will eventually get free. How often do you see

await streamController.close()

instead of just

streamController.close()

?

When using Flutter, often we even cannot await because we mostly free resources in widget state's void dispose() which is not awaited.

All too often we have this sync interface copied from one app to another and get re-declared in libs:

abstract class Disposable {
  void dispose();
}

Maybe we should just get it into the SDK? It alone would cover needs of most people reading this issue.

Next, we may use the fact that void method can be overriden with any return type, and make:

abstract class AsyncDisposable extends Disposable {
  @override
  Future<void> dispose();
}

So objects may expose void dispose() to agnostic code when handled as Disposable but still return Future to more coupled code that got them as AsyncDisposable and cares to await.

Having two interfaces (Disposable, AsyncDisposable) seems like it would just make things more complicated for everybody.

With such inheritance and a single method name, seems like it would make things more clear.

@lrhn
Copy link
Member

lrhn commented Dec 20, 2021

The problem here is that we can't even add dispose methods to existing classes. That's a breaking change. That's why an alias matching the existing cancel methods would be more suitable.

Also, making AsyncDisposable extend Disposable is problematic since it's usually not a good idea to ignore a returned future, but an AsyncDisposable cast to Disposable would encourage precisely that.
Being agnostic isn't working if ignoring the future is going to be an error.

@agrapine
Copy link

why do we need to unify the AsyncDisposable and Disposable?
the moment an async is involved the caller will most likely have to color its functions with async anyway.
But until it runs into an async situation for dispose it can already safely execute that.

I propose to scope out the sync&async dispose unification for an another issue, and start benefiting from having a Disposable contract. 🙏

@lrhn
Copy link
Member

lrhn commented Oct 26, 2023

If they were "unified", the other direction would likely work better:

// Don't implement this class, use one of the ones below.
abstract interface class PotentiallyAsyncDisposable {
  FutureOr<void> cancel();
}

abstract interface class AsyncDisposable implements PotentiallyAsyncDisposable {
  Future<void> cancel();
}

abstract interface class Disposable implements PotentiallyAsyncDisposable {
  void cancel();
}

Then a Disposable is-a PotentiallyAsyncDisposable (that completes quickly), but not the other way.

You need an AsyncDisposer to handle a PotentiallyAsyncDisposable (or how it's being handled), and its behavior is asynchronous, but a Disposer, possibly type-unrelated to AsyncDisposer, can handle plain Disposables synchronously.

@agrapine
Copy link

If they were "unified", the other direction would likely work better:

// Don't implement this class, use one of the ones below.
abstract interface class PotentiallyAsyncDisposable {
  FutureOr<void> cancel();
}

abstract interface class AsyncDisposable implements PotentiallyAsyncDisposable {
  Future<void> cancel();
}

abstract interface class Disposable implements PotentiallyAsyncDisposable {
  void cancel();
}

Then a Disposable is-a PotentiallyAsyncDisposable (that completes quickly), but not the other way.

You need an AsyncDisposer to handle a PotentiallyAsyncDisposable (or how it's being handled), and its behavior is asynchronous, but a Disposer, possibly type-unrelated to AsyncDisposer, can handle plain Disposables synchronously.

not quite sure why you would even try to do this.
lets say I'm in Flutter StatefullWidget dispose -> this won't be able to wait, so the dispose would have to be passed to something that can handle the async if necessary. making the code maybe correct, doesn't look like a good direction.

maybe this is more a Flutter topic than Dart.

@codelovercc

This comment was marked as off-topic.

@mraleph

This comment was marked as off-topic.

@MarvinHannott
Copy link

We can use generic Disposable.

void main() {
  final objA = D(A());
  final objB = D(B());
  final objC = D(C());

  objA.dispose();
  objB.dispose();
  objC.dispose();
}

abstract class Disposable<R> {
  R dispose();
}

class A implements Disposable<Future<int>> {
  @override
  Future<int> dispose() async {
    print('A disposed');
    return Future(() => 1);
  }
}

class B implements Disposable<void> {
  @override
  void dispose() {
    print('B disposed');
  }
}

class C {
  void dispose() {
    print('----');
  }
}

class D {
  final Object obj;

  D(this.obj);

  void dispose() {
    if (obj is Disposable) {
      (obj as Disposable).dispose();
    }
  }
}

Problem is: What if the constructor or dispose() throws? C# and Java guarantee proper destruction of every disposable object (if using using /try-with-resource) while Dart does not. This code, for example, would leak resources. And that's a huge problem.
Only way I see to guarantee proper destruction is to only ever have one (combined) Disposable at any given time and lots of discipline.

@TekExplorer
Copy link

@MarvinHannott
At that point i'd require that Object be a Disposable, and force the consumer to wrap it in a disposer

Frankly, i dont see why we cant have both, a Disposer implements Disposable which wraps objects that dont implement it, and Disposable itself for objects that do directly implement it.

This all makes me think of the whole serializable thing, which is very similar in concept, and could possibly benefit from the same logic

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

10 participants