[Catalyst-dev] Bug in Catalyst::Model::DBI .15

[Evan Carroll]

Let's start off with a brief description of the methods of M::DBI.
Currently, there is no connection to the database on ->new(). The
connection is established on the first call to ->dbh, when M::DBI
realizes it isn't connected, and proceeds to connect. This appears to
work, though it's really just a side-effect of a method
->stay_connected, being called prior to an official ->connect. This
works as long as $c->model('x') refers to the same instance, and NOT a
rebless of that compile-time instance.

The problem is with ACCEPT_CONTEXT and the current version of
Catalyst::Model::DBI.

You have to have a connection to the database, prior to having ->dbh
called. The most common use of ACCEPT_CONEXT looks like this:

sub ACCEPT_CONTEXT {
 my ( $self, $c ) = @_;
 bless { %$self, c => $c}, ref ( $self );
}

What this does is simple, it gives your model access to $c, and
ensures it's unique per request.

In this example, on each request, the object of an unconnected model,
is reblessed into a new object, where it establishes connection upon
the first call to ->dbh, unique from the compile-time copy of M::DBI.
In other words, on each call to $c->model('DBI')->dbh, a new
unconnected instance will have to connect, and because it's a rebless,
its connection won't be cached ( it will neither be the same for
another $c->model, nor will it exist outside of the one response. )

So if you call this twice in your Controller:
die $c->model('DBI')->dbh->{pg_pid}, $c->model('DBI')->dbh->{pg_pid}
OR if you use the power this provides to call another model, that
wraps around an M::DBI class, ex:

package M::DBI::FooTable;
sub create {
       my $self = shift;
       my $c = $self->{c};
       my $fkey_bar = $c->model('DBI::BarTable')->create(@_); ## this model
communicates on a different db session
}

You get failure.

I need this functionality because I use a different model for each
table, and on close inspection the reconnecting bit is a red flag. But
with the current implementation, you simply can not have cross table
transactions using the model-per-table method I've chosen.

With that said, the patch provided has no drawbacks, and even if you
only use the most minimal functionality of M::DBI, with the current
code, you don't have a connection until the first ->dbh is called,
which is no bueno. (work that is better handled at compile-time)

The grief was simply the nature of the problem, and having to
understand so much about the way the technology works to find the bug,
which isn't evident in most use cases.

On 5/5/07, Alex Pavlovic <alex at taskforce-1.com> wrote:
> Hi,
>
> On Friday 04 May 2007 19:01, Evan Carroll wrote:
> > M::DBI has a pretty sizable bug in as is such that it will establish a
> > new DBI handle on every incoming connection.
>
> In what environment CGI, FCGI, mod_perl ? You did not mention.
>
> > This is a problem for
> > speed, and transactions as it makes it much more difficult to share
> > $dbh across other models,
>
> If you want to share $dbh across multiple models then take a look at:
>
> http://search.cpan.org/author/ALEXP/Catalyst-Model-Proxy-0.03/lib/Catalyst/Model/Proxy.pm
>
> It has already been done.
>
> > or to wrap them. I'm very surprised this
> > went for such a long time undiscovered.
>
> What went on undiscovered ? I still dont understand what you are trying to
> accomplish. You can't share dbh across threads or multiple processes( because
> each connection results in distinct namespace ). DBIx::Class::Storage does
> the same thing, look at:
>
> http://search.cpan.org/~blblack/DBIx-Class-0.07006/lib/DBIx/Class/Storage/DBI.pm
>
> <code>
> elsif($self->_conn_pid != $$) {
>           $self->_dbh->{InactiveDestroy} = 1;
>           return $self->_dbh(undef);
> }
> </code>
>
> > This one line caused a lot of
> > grief.
>
> What kind of grief ?
>
> >
> > Thanks goes to >nothingmuch for all his help in first blaming the
> > problem totally on my style, and then later totally on M::DBI.
>
> I assume this was on irc. Next time please contact me before posting anything,
> it will save you some time and "grief". Not sure what your patch does here,
> maybe you can clarify.
>
> >
> > root at x60s:/usr/local/share/perl/5.8.8/Catalyst/Model# diff DBI.pm
> > DBI_FIX.pm 59a60,62
> >
> > >       $self->_dbh( $self->connect );
> >
> > 72c75
> > <                       } elsif ( $self->_pid != $$ ) {
> > ---
> >
> > >       } elsif ( $self->_pid != $$ ) {
> >
> > --
> > Evan Carroll
> > System Lord of the Internets
> > me at evancarroll.com
> > 832-445-8877
>
> --
> Alex Pavlovic - CTO
> TF-1 Inc. ( Custom development, consultancy and training )
> http://taskforce-1.com
>


-- 
Evan Carroll
System Lord of the Internets
me at evancarroll.com
832-445-8877