ap / DBIx-Connector

Fast, safe DBI connection and transaction management
https://metacpan.org/release/DBIx-Connector
40 stars 14 forks source link

Fix fixup #7

Closed yko closed 13 years ago

yko commented 13 years ago

Hi David!

I and one more developer - nuclon - found, that in fixup mode DBIx::Connector does not reconnect to database, as it expected.

After little investigation i've found that $dbh doesn't die on error, but returns undef as its described at http://search.cpan.org/~timb/DBI-1.615/DBI.pm#RaiseError

Test, i've midified may be not so obvious, so i provude real world test:

use strict;
use warnings;

use DBIx::Connector;

my $c = DBIx::Connector->new('dbi:mysql:test') or die "No DB connection";

print "First query: " . 
    $c->run(fixup => sub { shift->selectrow_array('SELECT 1')}), "\n";

sleep(10);

print "Second query: " . 
    $c->run(fixup => sub { shift->selectrow_array('SELECT 1')}), "\n";

This test written against mysql server and befor run it you have to set up appropriate timeout in your my.ini

[mysqld]

wait_timeout=5

And restart mysql server.

I think, similar test can be written against SQLite (removing sqlite file before second query) but i'm not sure.

Patch should solve the problem.

theory commented 13 years ago

Hrm. Can you not fix it by passing RaiseError => 1 to new()?

I'm loathe to accept this patch because not everyone uses RaiseError. I myself use HandleError. And maybe someone wants to turn it off for some reason.

yko commented 13 years ago

Hey, it's ok to reject my commit. Main message of it was that fixup method doesn't work as expected atm.

As only I have time I will try to find another way to catch error. Now I don't see other way to catch something like 'MySQL server has gone away' then set RaiseError or set HandleError which runs die or sets up $@.

Please note that you may misunderstood me: I don't pass 'RaiseError' to new in my commit. I just set local value of RaiseError that will be reverted at the end of block. Anyway this behavior of DBIx::Connect can be overloaded by user by globally setting HandleError to something, that returns true or by setting RaiseError to 0 in run() code block locally.

theory commented 13 years ago

Okay, this commit is my response to this issue. In short, DBIx::Connector will now set RaiseError to true in new() if neither it nor HandleError has been specified. But the documentation now covers this issue, stressing how important it is to set these attributes correctly in order to get proper exception handling from the execution methods.

Thanks!