Add analyzer warnings for classes holding FFI pointers to either release ownership or call the dispose method.

[Piero512]

Hi to the team.

Today I was writing a Flutter FFI library and it hit me: What if there was a way to annotate a class as holding native resources and this annotation reminds you that you needing to call some special method to manage the pointer inside.

Say, imagine I have a structure of a class like the following

@holdsNativeResources
abstract class  Disposable<Pointer<T>> {
    Pointer<T> ptr;

    Disposable(this.ptr);

    Pointer<T> releasePointer() {
        var copy = ptr;
        ptr = Pointer.nullptr;
        return copy;
    }

    void dispose() {
        <alloc>.free();
        ptr = Pointer.nullptr:
    }

}

And then, say, the analyzer would be able to tell if you haven't either released the pointer (ie. changed ownership of it) or disposed of it properly.

I know this work might conflict with the Dart finalizers, but I think that it would be a first step towards more-correct FFI usage.

[bwilkerson]

@dcharkes @cskau-g

[dcharkes]

And then, say, the analyzer would be able to tell if you haven't either released the pointer (ie. changed ownership of it) or disposed of it properly.

Dart does not have an ownership tracking type system like Rust does. How do you propose the Dart analyzer would know when to alert you?

[Piero512]

It would be great if it worked like Rust borrow checker, but right now I would like it to work like the don't forget to close sinks lint, maybe?

El jue., 22 de jul. de 2021 02:44, Daco Harkes @.***> escribió:

And then, say, the analyzer would be able to tell if you haven't either released the pointer (ie. changed ownership of it) or disposed of it properly.

Dart does not have an ownership tracking type system like Rust does. How do you propose the Dart analyzer would know when to alert you?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/dart-lang/sdk/issues/46649#issuecomment-884717391, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABSTWTG6OKITQW5ITBSBXWDTY7D7BANCNFSM5AQWS3XQ .

[dcharkes]

You mean a lint using the static scope? Like if you forget to do it before the end of a method body?

How about you use the Arena from package:ffi to do that instead as a coding pattern?

using((Arena arena) {
  // Allocate in arena.
  final pointer = arena<Int64>(2);

  // Use pointer.
});
// Everything in arena is freed.

[Piero512]

Yes, I'm already doing that, but the issue is with classes wrapping pointers to structs and such. I'll get some example code later to illustrate my point better.

El jue., 22 de jul. de 2021 05:23, Daco Harkes @.***> escribió:

You mean a lint using the static scope? Like if you forget to do it before the end of a method body?

How about you use the Arena from package:ffi to do that instead as a coding pattern?

using((Arena arena) { // Allocate in arena. final pointer = arena(2);

// Use pointer. });// Everything in arena is freed.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/dart-lang/sdk/issues/46649#issuecomment-884806663, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABSTWTABKMRXYT3N3PORQULTY7WR3ANCNFSM5AQWS3XQ .

[Piero512]

  static FrequenciesData loadFromAudioFile(String path) {
    const fftSize = 1024;
    const frameCount = fftSize;
    var arena = Arena(); // To avoid memory leaks
    var runningAverages = Float32List(fftSize);
    try {
      // Instantiates a ma_decoder struct and wraps it on this object.
      var decoder = MiniAudio.openDecoder(path); // I would like that the analyzer marked this if i'm missing a call to closeDecoder() or any other method I would use to free the C struct, or change ownership.
      // Getting the size of the frame in bytes AKA numChannels * sampleSize;
      var frameSize = decoder.frameSize;
      // My FFI returns -1 if it finds an error.
      assert(frameSize > 0, "Size can't be negative!");
      // Array for 256 frames (basically a holder array for the current samples)
      var sampleArray =
          arena.allocate(frameSize * frameCount, alignment: decoder.sampleSize);
      // Checks for the output format from MiniAudio
      var sampleFormat = decoder.decoder.ref.outputFormat;
      // nullable ptr. non-null if we need to do sample conversion (FFT lib uses float32).
      Pointer<Float>? sampleConvPtr;
      if (sampleFormat != ma_format.ma_format_f32) {
        sampleConvPtr = arena.call<Float>(frameCount);
      }
      var framesProcessed = 0; // Used to keep track
      // Used since miniaudio read_frames indicates EOF by returning you less that the requested amount of frames.
      var readFrames = 0;
      var fftFrameCount = 0;
      var sampleRate = decoder.decoder.ref.outputSampleRate;
      do {
        // Write frames to samplesArray.
        readFrames = decoder.readFrames(
            sampleArray.cast<Uint8>(), frameSize * frameCount, frameCount);
        framesProcessed += readFrames; // Add the recorded frames.
        late ArrayComplex fftResult;
        if (sampleConvPtr != null) {
          // If it needed conversion, convert it to f32
          extractF32Samples(sampleFormat, sampleArray.cast<Uint8>(), readFrames,
              sampleConvPtr);
          var samples = Array(sampleConvPtr.asTypedList(readFrames));
          fftResult = rfft(samples, n: fftSize);
        } else {
          // Else set the input to the window function to the raw samples.
          var samples = Array(sampleArray.cast<Float>().asTypedList(readFrames));
          fftResult = rfft(samples, n: fftSize);
        }
        // Transform Real FFT results to dBFS
        var absFft = arrayComplexAbs(fftResult);
        var normalizedFft = arrayDivisionToScalar(absFft, 0.5);
        var dbArray = arrayLog10(normalizedFft);
        var dbfs = arrayMultiplyToScalar(dbArray, 20);
        for(var i = 0; i < dbfs.length; i++){
          var divisor = fftFrameCount + 1;
          var newAverage = (runningAverages[i] * fftFrameCount + dbfs[i]) / divisor;
          if(newAverage <= double.negativeInfinity) {
            runningAverages[i] = -90;
          } else {
            runningAverages[i] = newAverage;
          }
        }
        fftFrameCount++;
      } while (readFrames == frameCount);
      decoder.closeDecoder();
      return FrequenciesData(
          sampleRate: sampleRate,
          data: runningAverages.take(fftSize ~/ 2).mapIndexed((i, e) => FrequencyIntensityPair(
                  getFrequency(i, sampleRate, fftSize), e))
              .toList());
    } catch (e) {
      print('Error received from library: $e');
      rethrow;
    } finally {
      arena.releaseAll();
      print("Should execute.");
    }
  }

This example is a little bit long as I'm actually writing this code for some audio processing in Flutter, but in order of easiest and most reachable would be:

• Annotate some of the class methods (that is the output of MiniDecoder.openDecode()) as either of those should be called, kinda the same way sinks do now (so that I mark closeDecoder() and releaseDecoderPtr() methods on that obj, and if I don't call either of them, then the analyzer warns me).
• Have some classes that implement ownership and/or refcount of the raw ptr's inside the class instances (maybe something like this) (please don't take this as something completely thought-out) :

  class MiniDecoder extends ffi.Refcounted {
  Pointer<ma_decoder_t> _decoder;
  MiniDecoder(this._decoder);
  @override
  List<Pointer> ownedPtrs => [_decoder];
  }

  If you're curious, the MiniAudio bindings that i'm using are in https://github.com/Piero512/miniaudio_decoder

[jacob314]

Would you still care about this lint if Dart providers https://github.com/dart-lang/sdk/issues/45455 a finalizer api suitable for FFI?

[Piero512]

To be honest, the proposal went a little bit over my head in how nuanced the situation was. I would probably not care about the lint if Dart provided an easy to use finalizer API, but it's definitively worth checking out.

[jcollins-g]

triaged as P3 not because it isn't potentially important, but because an analyzer change may not be the right way to solve the problem.

[bwilkerson]

I agree. That sounds more like a lint.