Condition inside a Transaction block that can raise "Bad state: This database has already been closed" Error

[aldycool]

Hi,

I think I found something that can raise a "Bad state: This database has already been closed" error when executing a certain call inside a transaction() block. This is the example:

await database.transaction(() async {
  // This is a normal insert inside a transaction
  await database
      .into(database.persons)
      .insert(PersonsCompanion(name: Value("A")));

  // This is a list that will be iterated using forEach(), and inserted into the table
  List<String> someCollection = [
    "X",
    "Y",
    "Z",
  ];

  someCollection.forEach((element) async {
    // The error is in here, because (I think) `forEach()` is actually executing a Function(), which is not compatible with Moor's `transaction()` block, see the `logStatements` output below
    await database
        .into(database.persons)
        .insert(PersonsCompanion(name: Value(element)));
  });
});

And the logStatements is like this:

Moor: Sent BEGIN TRANSACTION with args []
Moor: Sent INSERT INTO person (id, name, birthday) VALUES (?, ?, ?) with args [3dec03ac-c925-4f39-910b-3017a3c3c36e, A, 1626932081]
Moor: Sent COMMIT TRANSACTION with args []
Moor: Sent INSERT INTO person (id, name, birthday) VALUES (?, ?, ?) with args [f9a7ef33-1fb2-46b2-9952-8e29e63d9e73, X, 1626932081]
Moor: Sent INSERT INTO person (id, name, birthday) VALUES (?, ?, ?) with args [44c07f7e-e29d-4364-8969-0759bddec1fe, Y, 1626932081]
Moor: Sent INSERT INTO person (id, name, birthday) VALUES (?, ?, ?) with args [50715bc1-1170-4734-8ed2-de8da7d697d2, Z, 1626932081]

See that Moor is sending COMMIT TRANSACTION before the collection inserts are complete. I think this is caused by the forEach() behavior which is executing Function() that is not wrapped by the transaction() block. I don't know if this issue has been raised before (I've searched the Issues and cannot find related ones), and I thought I just share it here just in case we've missed it. Is this issue is actually has been addressed or known? if so, is there any "gotchas" (beside the await and Stream that are mentioned in the docs) that I should know about? Thanks!

[simolus3]

Essentially, this is a misuse of the transaction api. Moor won't be able to fix this, but we should at least report a much better error message in this case.

As you mentioned, using an async callback in forEach will schedule async work without awaiting it. The transaction will be closed after call to forEach, but before all of the insert calls are completed.
The workaround in this case is to use await Future.wait(someCollection.map((element) async {})), which will wait for everything to complete.

To avoid this confusion, moor should add checks throwing an error if a completed transaction is still being used.

[simolus3]

And it's fixed in 6e192fb. I've opted to only run this check when assertions are enabled because it's unlikely that code will work in debug builds but misbehave in releases. So we get the better error message without additional costs.

[aldycool]

Yes I"ve seen the changes. The approach looks good, and should cover how most people will encounter this. Thanks, and nice work.