dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.28k stars 1.58k forks source link

Disposable #43490

Open vsevolod19860507 opened 4 years ago

vsevolod19860507 commented 4 years ago

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

lrhn commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

I'm understood, thank you.

themisir commented 3 years ago

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 commented 3 years ago

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

What about FutureOr<void> dispose()?

lrhn commented 3 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 1 year ago

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. :pray:

lrhn commented 1 year ago

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 commented 1 year ago

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 commented 10 months ago

Why wouldn't you make sync method be able to wait async method? Like Future.wait()

void dispose(){
    closeSomethingAsync().wait();
}

Futrue<void> closeSomethingAsync() async(){
   ...
}
mraleph commented 10 months ago

@codelovercc

Why wouldn't you make sync method be able to wait async method? Like Future.wait()

This question is off-topic for this issue.

But tldr is that:

See https://github.com/dart-lang/sdk/issues/52121 and https://github.com/dart-lang/sdk/issues/39390

MarvinHannott commented 7 months ago

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 commented 4 months ago

@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