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

Weird side effects with DriftIsolate.spawn #2775

Closed
FaFre opened this issue Dec 7, 2023 · 4 comments
Closed

Weird side effects with DriftIsolate.spawn #2775

FaFre opened this issue Dec 7, 2023 · 4 comments

Comments

@FaFre
Copy link
Contributor

FaFre commented Dec 7, 2023

Today I migrated one of my databases to run in the background with DriftIsolate.spawn. This database getting freshly created in memory every app-start and filled with data which is shipped as an asset.

To spin up the database I do something like this:

DatabaseConnection _backgroundConnection() {
  return DatabaseConnection(NativeDatabase.memory());
}

class DatabaseImplementation implements IDatabaseImplementation {
  @override
  Future<DatabaseConnection> openInMemory() async {
    return DriftIsolate.spawn(_backgroundConnection)
        .then((value) => value.connect());
  }
}

//set up db
PresetDatabase.lazy(
    Future<QueryExecutor> Function() e, {
    this.onCreateStatements,
  }) : super(LazyDatabase(e));

...

return PresetDatabase.lazy(
    () async => DatabaseImplementation().openInMemory(),
    onCreateStatements: () async =>
        rootBundle.load('assets/database/preset_data.sql.gz').then(
              (byteData) async => compute(
                (message) => utf8.decode(
                  GZipDecoder().decodeBytes(message.buffer.asUint8List()),
                ),
                byteData,
              ),
            ),
  );

I used to have following initialization:

  @override
  MigrationStrategy get migration => MigrationStrategy(
        onCreate: (Migrator m) async {
          await m.createAll();

          if (onCreateStatements != null) {
            await onCreateStatements!().then((m.database.connection.runCustom));
          }

          if (initializeSearchIndex) {
            await (m.database as PresetDatabase).createSearchIndex();
          }
        },
      );

The migration worked completely fine when using without isolate. However, with isolate the above is completely faulted. There are no exceptions thrown nor any other odds are happening.

The following future will never finish, and I have no explanation why:
await onCreateStatements!().then((m.database.connection.runCustom));

Funny thing is, this is working without problems:

final statements = await onCreateStatements!();
m.database.connection.runCustom(statements);

Statements is a string with ~7MB in size, so its pretty huge.

So, now after I fixed this issue, following is messed up:
await (m.database as PresetDatabase).createSearchIndex();

Which is defined as this:

createSearchIndex:
  INSERT INTO 
    preset_term_fts (rowid, term) 
  SELECT 
    rowid, 
    term
  FROM preset_term;

It will return 10 affected rowids, but it should insert ~170k rows.

I also have a fix for that, calling it after database creation like:

final db = PresetDatabase.lazy(...);
await db.createSearchIndex();

Unfortunately I don't have the time today to provide a complete example. I can follow up with that following days if required.

Are there any explanations? I also tested it in release mode since I experienced bugs with isolates in debug in past flutter releases. However, these bugs persist in release as well.

@simolus3
Copy link
Owner

simolus3 commented Dec 7, 2023

I can reproduce this issue with a very small string (echo "CREATE TABLE foo (bar)" | gzip > preset_data.sql.gz) and the following script

import 'dart:convert';
import 'dart:io';
import 'dart:isolate';

import 'package:drift/drift.dart';
import 'package:drift/isolate.dart';
import 'package:drift/native.dart';

DatabaseConnection _backgroundConnection() {
  return DatabaseConnection(NativeDatabase.memory());
}

class DatabaseImplementation {
  @override
  Future<DatabaseConnection> openInMemory() async {
    return DriftIsolate.spawn(_backgroundConnection)
        .then((value) => value.connect());
  }
}

class MyDatabase extends GeneratedDatabase {
  final Future<String> Function()? onCreateStatements;

  MyDatabase.lazy(
    Future<QueryExecutor> Function() e, {
    this.onCreateStatements,
  }) : super(LazyDatabase(e));

  @override
  Iterable<TableInfo<Table, dynamic>> get allTables => const Iterable.empty();

  @override
  int get schemaVersion => 1;

  @override
  MigrationStrategy get migration => MigrationStrategy(
        onCreate: (Migrator m) async {
          await m.createAll();

          if (onCreateStatements != null) {
            await onCreateStatements!().then((m.database.connection.runCustom));
          }
        },
      );
}

void main() async {
  final file = File('preset_data.sql.gz');

  final db = MyDatabase.lazy(
    () async => DatabaseImplementation().openInMemory(),
    onCreateStatements: () async => file.readAsBytes().then(
          (byteData) async =>
              Isolate.run(() => utf8.decode(gzip.decode(byteData))),
        ),
  );

  print(await db.customSelect('select 1').get());
}

Interestingly, using await onCreateStatements!().then(customStatement); works. Could you try and let me know if that fixes the index issue as well? Otherwise I'd probably need more information about the table structure and the init string to reproduce this.

@simolus3
Copy link
Owner

simolus3 commented Dec 7, 2023

Oh wait I know what's going wrong.

While a migration is running, the rest of the database is blocked. This is because we only open a database and start the migration in response to the first query, which obviously shouldn't start to run before the migration has completed. But when you're using connection.runCustom, that runs on the top-level database connection. So you have a deadlock there because the migration is waiting for the top-level connection to unlock, which in turn is waiting for the migration.

Using customStatement fixes it because it looks up the current context from the async zone and knows to use the unlocked connection for the migration.

Using

final statements = await onCreateStatements!();
m.database.connection.runCustom(statements);

works because you're not awaiting runCustom, so it only starts to run after the migration. That could explain why the search index hasn't been indexed, the data isn't there when createSearchIndex is called in the migration.

@FaFre
Copy link
Contributor Author

FaFre commented Dec 7, 2023

Thank you a lot @simolus3!

Indeed, runCustom fixes the problem and everything got imported nicely. I remeber now I was a bit scared of using runCustom because it says

[statement] should contain exactly one SQL statement. Attempting to run multiple statements with a single [customStatement] may not be fully supported on all platforms.

And I'am executing thousands at once. But now rethinking it this warning is probably because of the parameter binding (which I'm not using)?

@FaFre FaFre closed this as completed Dec 7, 2023
@simolus3
Copy link
Owner

simolus3 commented Dec 8, 2023

And I'am executing thousands at once. But now rethinking it this warning is probably because of the parameter binding (which I'm not using)?

Yes, it shouldn't be a problem in this case - and customStatement actually just calls runCustom under the hood without any changes to the SQL.

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

2 participants