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

Auto-Free of Pointers and other natively allocated memory #1642

Open
rmtmckenzie opened this issue May 20, 2021 · 4 comments
Open

Auto-Free of Pointers and other natively allocated memory #1642

rmtmckenzie opened this issue May 20, 2021 · 4 comments
Labels
request Requests to resolve a particular developer problem

Comments

@rmtmckenzie
Copy link

rmtmckenzie commented May 20, 2021

(I thought there was already an issue about this, but I couldn't find it after a search, so I've opened one. If there is one already, sorry about that, and I'll close this right away!)

I was watching the session on FFI from Google IO and something caught my mind - the fact that it would
be extremely easy to miss the "free" on the pointer they created. I've been using FFI in a few internal projects, and wrote some tooling around freeing to make it easier to ensure memory was freed.

I realize that using anyone using FFI should be familiar with this sort of thing and be very careful, but the fact of the matter is that this sort of thing happens - which is a big part of why modern C++ uses things like unique_ptr, and why rust was created in the first place. Even the Dart FFI samples aren't even immune to this, and you'd hope that they would always have been correct.

I see an opportunity here while FFI is still relatively recent, to do something that helps with this. Namely, a try-with-resources type construct would encourage people to handle memory management in a saner way. In addition to this, it would be wonderful if a dart analysis option were added to warn for unsafe usages of Pointer et al (and with a warning suppression for special cases). That might not find all potential memory leaks but would certainly be a good start. It'd be great if it were able to handle some of the basic structures provided by FFI as well as objects implementing an interface, say something like Freeable.

// this should provoke a warning with the analysis_option 'unsafe_pointers'
final hello = 'hello'.toNativeUtf8();
reverse(hello);
print(hello.toDartString());
malloc.free(hello);

An example syntax could be:

with (hello = 'hello'.toNativeUtf8()) {
  reverse(hello);
  print(hello.toDartString());
}

(I omitted 'final' before final hello = ... - I don't think there's a situation here where you'd want to use a var for hello as that seems like it'd be asking for problems. Defining the variable type might be nice, although I think it'd still have to stay final).

I realize that freeing isn't always that simple, so maybe in some contexts you might have to write your own freeing code, which would place the responsibility back on the programmer but would assume that they know what they're doing.

with (hello = 'hello'.toNativeUtf8()) {
  reverse(hello);
  print(hello.toDartString());
} finally {
  malloc.free(hello);
}

If possible technologically, this should also be useable in asynchronous functions - either by default, or with the async keyword applied, i.e.

with (hello = 'hello'.toNativeUtf8()) async {
  reverse(hello);
  final name = await retrieveName(hello.toDartString());
  print("$name ${hello.toDartString()}");
}

I do realize that this could can be done with a function and try/finally - but I really think that the more important part of this issue is the ability to enforce the standard and have people use it consistently.

This is basically what I've done in my own projects using FFI:

abstract class Freeable {
  void free();
}

R autoFree2<T1 extends Freeable, T2 extends Freeable, R>(T1 a1, T2 a2, Future<R> Function(T1 a1, T2 a2) operation) async {
  try {
    return await operation(a1, a2);
  } finally {
    a2.free();
    a1.free();
  }
}

While this works, it is obviously not optimal for many reasons - namely that any ffi primitives I use have to be wrapped in a "Freeable" object (although this could be simplified if Extensions were able to implement interfaces), and that I either have to define one of these for each number of arguments, use many wrapping each other, or not use generics and resort to (less safe) down-casting of types.

References:

@rmtmckenzie rmtmckenzie added the request Requests to resolve a particular developer problem label May 20, 2021
@leafpetersen
Copy link
Member

cc @mraleph @dcharkes

@dcharkes
Copy link
Contributor

We should merge our pool sample to package:ffi so that everyone can use the same using((Pool pool) { ... } pattern. Simple examples:

  // To ensure resources are freed, wrap them in a [using] call.
  using((Pool pool) {
    final p = pool<Int64>(2);
    p[0] = 24;
    MemMove(p.elementAt(1).cast<Void>(), p.cast<Void>(), sizeOf<Int64>());
    print(p[1]);
    Expect.equals(24, p[1]);
  });

  // Resources are freed also when abnormal control flow occurs.
  try {
    using((Pool pool) {
      final p = pool<Int64>(2);
      p[0] = 25;
      MemMove(p.elementAt(1).cast<Void>(), p.cast<Void>(), 8);
      print(p[1]);
      Expect.equals(25, p[1]);
      throw Exception("Some random exception");
    });
    // `calloc.free(p)` has been called.
  } on Exception catch (e) {
    print("Caught exception: $e");
  }

The pool also works with async and zones.

We have some proposals for various syntax additions (internal doc). But so far we have settled with the Pool without any syntax additions for syntactic scope lifetimes of native resources.

(Automatic freeing without using syntactic scope but with object lifetime is a separate discussion: dart-lang/sdk#35770 https://github.com/dart-lang/sdk/issues/45455)

@rmtmckenzie
Copy link
Author

That seems pretty useable and definitely more flexible and clean than what I've been doing; if it were merged to somewhere more useable and the ffi documentation updated to mention/recommend it I see no problem with it being the recommended solution, so long as there is a solution.

The ability to turn on an analyzer flag to prevent unmanaged use of pointers would still be great though; even with some big caveats that might have to apply, I think it would be a big win and likely easier to implement than something like finalizers & automatic freeing. I guess a good question would be whether that is be possible with this Zone implementation (or even something that would be considered). While I have the utmost respect for package maintainers in general and especially some of the very quality packages on pub.dev, mistakes do happen especially around this memory management.

@dcharkes
Copy link
Contributor

@rmtmckenzie We've published package:ffi 1.1.0 with an Arena allocator (we renamed Pool to Arena).

https://pub.dev/packages/ffi/versions/1.1.0

Feel free to leave your feedback or improvements on the package repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
request Requests to resolve a particular developer problem
Projects
None yet
Development

No branches or pull requests

3 participants