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

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

Open
yahermann opened this issue Dec 15, 2015 · 2 comments

Comments

@yahermann
Copy link

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

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.

@yahermann yahermann changed the title No rollback on die when AutoCommit=1 and RaiseError=1 No rollback if thread ends during transaction; poisoned handles end up in connection pool Jan 1, 2016
@bigpresh
Copy link
Owner

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.

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