drift icon indicating copy to clipboard operation
drift copied to clipboard

Potential Memory Leak when opening and closing connections & lazy database closure

Open devroble opened this issue 10 months ago • 10 comments

A potential memory leak occurs when repeatedly opening and closing connections in Drift. Instances of DriftCommunication accumulate over time and are not properly deallocated, leading to increasing memory usage. This issue suggests that resources tied to DriftCommunication are not being released when a connection is closed. NotifyTablesUpdated and TableUpdate are also affected.

Closing a lazy database without performing any operations beforehand results in an increasing number of isolates, suggesting a resource leak as well.

Steps to Reproduce:

  • Open a connection to a Drift database.
QueryExecutor openConnection(String dbPath) {
  return driftDatabase(
    name: dbPath,
    native: DriftNativeOptions(
      shareAcrossIsolates: false,
      databasePath: () async {
        return join(await getDatabasesPath(), dbPath);
      },
    ),
  );
}
  • Close the connection.
  • Repeat the open/close cycle multiple times.
  • Monitor memory usage or track DriftCommunication instances.

Image

Using:

  • flutter: 3.24.5
  • drift: 2.23.1
  • drift_flutter: 0.2.4

devroble avatar Feb 02 '25 18:02 devroble

Thanks for the report! I've tried to reproduce it, but unfortunately wasn't successful. I've used this program:

void main() async {
  FlutterError.demangleStackTrace = (StackTrace stack) {
    if (stack is Trace) return stack.vmTrace;
    if (stack is Chain) return stack.toTrace().vmTrace;
    return stack;
  };

  for (var i = 0; i < 10; i++) {
    final db = AppDatabase(driftDatabase(
      name: 'repro.db',
      native: DriftNativeOptions(
        shareAcrossIsolates: false,
        databasePath: () async => '/tmp/test.db',
      ),
    ));

    await db.customStatement('SELECT 1;');
    await db.close();
  }

  print('at end');
}

Setting a breakpoint on the print line at the end, opening devtools and starting one manual GC only shows a single DriftCommunication object afterwards.

simolus3 avatar Feb 02 '25 20:02 simolus3

Thanks for the fast response! I appreciate your time in trying to reproduce the issue.

With your example, I also couldn't reproduce the issue. However, I forgot to mention an important detail...I am using Drift in combination with workmanager (v.0.5.2), where the database is opened and closed within a background task.

It’s possible that the issue only occurs in this specific setup. Could you try testing it in a workmanager background task to see if the behavior changes?

devroble avatar Feb 02 '25 22:02 devroble

For the second issue (lazy database closure causing isolate leaks), I was able to reproduce it with your program if I remove the following line: await db.customStatement('SELECT 1;');

Without this statement, the number of isolates increases each time the database is opened and closed.

Image

devroble avatar Feb 03 '25 05:02 devroble

Good point, thanks! I've fixed that in 49ebf8828755283d43bcd00906c437a0c11594dd. I'm not convinced that issue is what you were seeing with workmanager since I assume you probably used the database there. To help me reproduce that, could you share how you're closing the database in the workmanager task? If the main database class isn't garbage collected (e.g. due to an unrelated leak, it's also possible for the other instance to be tied to that). Is the database opened within the task and not used anywhere else?

simolus3 avatar Feb 03 '25 20:02 simolus3

Oh well, as you suggested, I forgot to close the database, so it remained open and unused. After properly closing the database in the WorkManager task, the memory leak is now fixed.

Thanks for pointing it out!

devroble avatar Feb 04 '25 10:02 devroble

We also had an aggregation of DriftCommunication instances. While this problem was fixed by calling db.close() as described, we still have an aggregation of NotifyTableUpdated and TableUpdate instances:

Image

@devroble As I can see in your debugger-screenshot you also had an aggregation of these instances. Do you still have the same problem in your case?

johannessachse avatar Feb 06 '25 13:02 johannessachse

I couldn't reproduce that problem in a small demo that listens to stream queries and then closes an isolate database repeatedly. Are you listening to table updates manually or is this just through stream queries?

simolus3 avatar Feb 08 '25 14:02 simolus3

We also had an aggregation of DriftCommunication instances. While this problem was fixed by calling db.close() as described, we still have an aggregation of NotifyTableUpdated and TableUpdate instances:

Image [@devroble](https://github.com/devroble) As I can see in your debugger-screenshot you also had an aggregation of these instances. Do you still have the same problem in your case?

Good observation, @johannessachse ! Yes, the aggregation of instances still exists, so there does seem to be an issue. While closing the database in the WorkManager task fixed one part of the problem, the accumulation of NotifyTableUpdated and TableUpdate instances is still happening. Are you setting shareAcrossIsolates: true as well?

I assume, that this then()-part in server_impl.dart seems not to be triggered in my case:

  /// Creates a server from the underlying connection and further options.
  ServerImplementation(this.connection, this.allowRemoteShutdown,
      this.closeExecutorWhenShutdown) {
    done.then((_) {
      _closeRemainingConnections();
      _tableUpdateNotifications.close();
    });
  }

Retaining path for NotifyTableUpdated:

dart:async/_DelayedData
dart:async/_PendingEvents
dart:async/_AsyncStreamController
package:drift/src/remote/server_impl.dart/ServerImplementation
package:drift/src/isolate.dart/RunningDriftServer
Closure Context
dart:core/_Closure
dart:async/_ControllerSubscription
dart:async/_SyncStreamController
dart:isolate/_RawReceivePort
dart:core/_List
dart:_compact_hash/_Map
Isolate

Retaining path for TableUpdate:

dart:core/_List
dart:core/_GrowableList
package:drift/src/remote/protocol.dart/NotifyTablesUpdated
dart:async/_DelayedData
dart:async/_PendingEvents
dart:async/_AsyncStreamController
package:drift/src/remote/server_impl.dart/ServerImplementation
package:drift/src/isolate.dart/RunningDriftServer
Closure Context
dart:core/_Closure
dart:async/_ControllerSubscription
dart:async/_SyncStreamController
dart:isolate/_RawReceivePort
dart:_compact_hash/_Map
Isolate

devroble avatar Feb 09 '25 21:02 devroble

Hi, I work with Johannes and since he is on a well deserved vacation, I will try to pick this up.

@devroble We are not using the drift_flutter & shareAcrossIsolates since our setup is older. We manually share the port. But I think our setup pretty much reflects the same usage.

The retain paths seem to indicate that this is being retained on the (drift) server side (drift isolate)., right?

@simolus3 Not sure how to reproduce this in a small sample.

kuhnroyal avatar Feb 10 '25 15:02 kuhnroyal

Should we create another issue or reopen this issue?

devroble avatar Feb 14 '25 07:02 devroble

For the second issue (lazy database closure causing isolate leaks), I was able to reproduce it with your program if I remove the following line: await db.customStatement('SELECT 1;');

Without this statement, the number of isolates increases each time the database is opened and closed.

Image

I'm sure if this is the same issue, but I get multiple instances of the "Drift isolate worker for /path/to/database.sqlite".

Steps to reproduct:

  1. On Android enable "Don't keep activities" in Developer options
  2. Run the example app in example/app (I did it from VSCode)
  3. Sent the app to the background and resume.
  4. You should now have a second "Drift isolate worker"
  5. Sent the app to the background again and resume.
  6. You should now have a third "Drift isolate worker"

m-abs avatar Jul 07 '25 14:07 m-abs

Thanks for that report, that's interesting to know! I don't think it's related to this issue, but it's something we can fix in drift anyway.

simolus3 avatar Jul 08 '25 21:07 simolus3