No rollback if thread ends during transaction; poisoned handles end up in connection pool

[yahermann]

I use the following configuration:

my $db = {
    driver => "mysql",
    host => "localhost",
    database => 'mydb',
    dbi_params => {
      mysql_enable_utf8 => 1,
      charset => "utf8",
      RaiseError => 1,
      PrintError => 0,
      AutoCommit => 1,
      }
}

The above is defined globally as described here: https://github.com/bigpresh/Dancer-Plugin-Database/issues/75

For single-statement write transactions I just rely on AutoCommit=1 and RaiseError=1 to automatically commit each transaction and to automatically raise any errors.

  my $dbh = database($db);
  $dbh->do("SQL WRITE 1 of 1");

This works very well. If there's a problem, the app dies and the error is reported nicely by Dancer. No need to clutter up the code with ... or die $dbh->errstr everywhere.

The problem occurs with transactions. The pattern I use is:

  my $dbh = database($db);
  $dbh->begin_work();
  $dbh->do("SQL WRITE 1 of 3");
  $dbh->do("SQL WRITE 2 of 3");
  $dbh->do("SQL WRITE 3 of 3");
  ...
  $dbh->commit();

If there's a problem with any of the SQL writes within the transaction block, or some other exception gets thrown within the transaction, the database handle (with the uncommitted, unrolledback transactions) gets returned back to the connections pool within the plugin. That handle then eventually gets picked up by another thread later, and all sorts of really bad things happen.

It seems to me the plugin should somehow check the database handle when it's returned to the pool, and if it contains a pending transaction, $dbh->rollback(); should be invoked.

Note that just detecting die at end of thread execution would not be enough, because it wouldn't prevent a sloppy coder from poisoning the connection pool like this:

$dbh->begin_work();
$dbh->do("something");
$dbh->do("something else");
...
exit;  # normal exit, but no $dbh->commit(); or $dbh->rollback();

I'm not sure if this is in the category of "bug/issue", or "wishlist", but it seems to me it's the former because the plug-in is allowing bad handles back into the pool.

Workaround: To get around this issue, I started using the following pattern which so far appears to work:

eval {
   $dbh->begin_work();  # this should probably be prior to eval block... ???    
   $dbh->do('something here');
   ... other stuff here....
   $dbh->commit();
   1;  # because $@ is unreliable
    }
or do {
   my $err = $@;   # is this the best we can do for a good error msg, even though it's unreliable?
  $dbh->rollback();
   die $err;
   };

I'm not thrilled about repeating the above code, over and over again, throughout my app. I'm lazy and sloppy.

I'm very open to suggestions/improvements on the above workaround.

[yahermann]

Doing a little more research on the feasibility of improving the plugin by reading the state from DBI, I think I've hit a brick wall. Apparently there is a $dbh->{begunwork} but it's undocumented/unsupported:

http://stackoverflow.com/questions/1980833/how-can-i-test-if-the-dbi-driver-state-is-within-a-transaction

Given the uncertain nature of relying on a documented feature, one option might be for the plugin to intercept the begin_work(), rollback(), and commit() methods, and definitively implement the begunwork option above within the plugin. Then when a dbh is returned to the pool, $dbh->rollback() if $dbh->{begunwork}, prior to assigning the handle to a new thread.

[bigpresh]

Sorry for the lack of response here. I had a think, but hit a mental brick wall and had an oh look a squirrel moment. Yeah, you're quite probably right that catching begin_work(), rollback() etc calls and keeping state there could work. However - there isn't really a concept of returning handles to a pool to give to another thread - it's one handle per process/thread, and therefore it's the same handle that that same process gets later.

The problem comes with what happens if you say e.g.:

database->begin_work();
database->do(...);
database->do(...);
...

... rather than grabbing the handle and storing it yourself briefly.

Each of those calls fetches the same handle, but how can the plugin know if you're expecting it to have uncommitted changes or not?

I suspect a suitable solution may be for the plugin to add an after hook handler which looks for any active DB handles with uncommitted changes, bitches and rolls them back - but to detect those uncommitted changes, we'd probably need to do what you said in hooking begin_work, rollback and commit if $dbh->{begunwork} can't be relied upon. Otherwise, it could do that check each time you fetch a handle, and we could document that that will happen, and that if you need to do a transaction you just grab the DB handle yourself and assign it to a variable.