Linter: Suggestion - consider adding checks for issues causing a process with isolates to hang

[annagrin]

Apologies if the check already exists - please let me know. I, like some others before me, run into a non-terminating isolate problems that took me a few days to detect and correct. Currently, there is no easy way to debug non-terminating isolates and detect what is holding them running. At least in my case, the bugs seemed to be relatively easy to detect statically. Note that this is not a general check suggestion for closing all handles - it aims only at ones that can hang the process, and at code patterns people use for them (there could be more types of those bugs, of course).

1. in package:http/http.dart, Client.close() has the following comment:

  /// Closes the client and cleans up any resources associated with it.
  ///
  /// It's important to close each client when it's done being used; failing to
  /// do so can cause the Dart process to hang.
  void close();

Indeed, I run into a problems where a program wouldn't terminate because it was using a hander with a local client variable that was never closed. If the general form of this analysis is noisy, there are some very simple cases we could detect where the users could save a lot time and debugging work:

Handler proxyHandler(url, {http.Client client, String proxyName}) {
  /* code not using client */

  client ??= http.Client();

  /* code using client */
  // bug - client is not closed
}

Another case could be a field in a class that have a close() method that is never called in the class (at least for non-virtual calls).

1. Similarly, any receive ports that are not closed when running an isolate (on either the main isolate or the spawn one) will cause the process to not terminate. Some simple patterns could be detectable:

Spawn Isolate:

Future main(List<String> args, [SendPort sendPort]) async {
  if (sendPort != null) {
    var receivePort = ReceivePort();
    sendPort.send(receivePort.sendPort);
    var worker = await Worker.create(
          requestStream: receivePort.cast<Map<String, dynamic>>(),
          sendResponse: sendPort.send);
    await worker.start();
    // receivePort.close(); // bug - port is not closed!
  }
}

Main isolate:

class Service {
  final ReceivePort _receivePort;
  final SendPort _sendPort;

  Service._(
    this._responseStream,
    this._receivePort,
    this._sendPort) {
   _responseStream.listen((response) {
      // handle responses
    });
  }

  static Future<Service> start() async {
    final receivePort = ReceivePort();
    final isolate = await Isolate.spawnUri(
      workerUri,
      args,
      receivePort.sendPort,
    );

    // Wait to get the sendPort from the isolate
    var stream = receivePort.asBroadcastStream();
    var sendPort = await stream.first as SendPort;

    return Service._(stream, receivePort, sendPort);
  }

  /* code that sends requests  not shown */ 

  Future<void> stop() async {
     // _receivePort.close(); // bug - _receivePort field is not closed anywhere in this class.
  }
}

[kevmoo]

I wonder if we need a new annotation in package meta. @requireDispose or similar.

[bwilkerson]

Yes, an annotation would be the most general way to solve the problem and would allow any package author to specify a method that must be called (and presumably called last?).

However, a complete solution -- catching every possible case where an instance isn't closed -- isn't possible. That isn't to say that we couldn't catch a reasonable percentage of cases, or that we shouldn't do what we can, just that we'd need to make it clear that our solution isn't perfect.

[kevmoo]

we'd need to make it clear that our solution isn't perfect.

As long as it's as good as the current Stream solution, I think that's a great start!