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

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

Open
devroble opened this issue Feb 2, 2025 · 10 comments

Comments

@devroble
Copy link

devroble commented Feb 2, 2025

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
@simolus3
Copy link
Owner

simolus3 commented Feb 2, 2025

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 simolus3 added the needs-info More information is needed to act on the issue label Feb 2, 2025
@devroble
Copy link
Author

devroble commented Feb 2, 2025

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
Copy link
Author

devroble commented Feb 3, 2025

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

@simolus3 simolus3 removed the needs-info More information is needed to act on the issue label Feb 3, 2025
@simolus3
Copy link
Owner

simolus3 commented Feb 3, 2025

Good point, thanks! I've fixed that in 49ebf88. 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?

@devroble
Copy link
Author

devroble commented Feb 4, 2025

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!

@simolus3 simolus3 closed this as completed Feb 4, 2025
@johannessachse
Copy link

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?

@simolus3
Copy link
Owner

simolus3 commented Feb 8, 2025

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?

@devroble
Copy link
Author

devroble commented Feb 9, 2025

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

@kuhnroyal
Copy link
Contributor

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.

@devroble
Copy link
Author

Should we create another issue or reopen this issue?

@simolus3 simolus3 reopened this Feb 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants