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

Sun May 6 22:45:25 GMT 2007

Hi,

Fix ( as per Aristotle Pagaltzis suggestion ) has been tested/commited into 
svn and on pause ( cpan ).

Please note that each model will still create single initial connection ( when 
request for $dbh arrives ). This happens with either approach ( squirreling 
away a connection in "new" or ref to undef scalar ).

However re-connect will not be made afterwards for each new request. This is 
where the C::M::Proxy has advantage because single connection is created and 
maintained for all models. I would strongly recommend that you use it, rather 
then mucking around with ACCEPT_CONTEXT.

Enjoy.

On Sunday 06 May 2007 12:38, Alex Pavlovic wrote:
> Hi,
>
> On Sunday 06 May 2007 09:37, Evan Carroll wrote:
> > > 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.
>
> It's not a side effect, it's called deferred initiliazation.
>
> > > This
> > > works as long as $c->model('x') refers to the same instance, and NOT a
> > > rebless of that compile-time instance.
>
> Every model in catalyst that you have is a new instance, therefore it
> results in new connection, if this is what you meant. It is obvious
> re-connect will be made, because $dbh is stored with each original
> instance.
>
> This is precisely why I wrote the proxy class, where operations are just
> being forwarded to one centralized instance that has them cached and you
> need none of the ACCEPT_CONTEXT magic.
>
> > > The problem is with ACCEPT_CONTEXT and the current version of
> > > Catalyst::Model::DBI.
>
> While this is a problem, you have to realize that this is still only a
> problem for people who use the model in such way that you do. I will work
> towards a patch so that it may satisfy your needs. If you are inclined or
> have already started any work, please inform me directly.
>
> > > 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.
>
> You can, but not the way you are doing it at the moment.
>
> > > 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.
>
> Please understand that this is community driven project. Nothing is
> perfect. Lot of us here dedicate our free time to work on these things.
>
> > > 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/Cat
> > > >al yst/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/Sto
> > > >ra ge/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
> > >
> > >
> > >
> > > ------------------------------
> > >
> > > Message: 2
> > > Date: Sun, 6 May 2007 11:29:48 +0200
> > > From: "A. Pagaltzis" <pagaltzis at gmx.de>
> > > Subject: [Catalyst-dev] Re: Bug in Catalyst::Model::DBI .15
> > > To: catalyst at lists.rawmode.org, catalyst-dev at lists.rawmode.org
> > > Message-ID: <20070506092948.GN20785 at klangraum>
> > > Content-Type: text/plain; charset=utf-8
> > >
> > > * Evan Carroll <lists at evancarroll.com> [2007-05-06 10:55]:
> > > > 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)
> > >
> > > That is precisely the patch's drawback. Connecting lazily rather
> > > than eagerly is a feature.
> > >
> > > However, that's not hard to correct, although the patch would be
> > > larger: rather than squirreling away a connection in `new`,
> > > squirrel away a reference to an undef scalar. Then you can assign
> > > the handle to that scalar, and it will be shared among everyone
> > > who kept a copy of the same reference.
> >
> > I don't see eye to eye with you on this one, knowing if you fail to
> > connect to a DB prior to pulling up a page that requires it seems like
> > a good functional bonus. On the flip side, I'm less concerned with
> > this minor detail than the larger issue of getting it fixed.
> >
> > > --
> > > *AUTOLOAD=*_;sub _{s/(.*)::(.*)/print$2,(",$\/"," ")[defined
> > > wantarray]/e;$1} &Just->another->Perl->hack;
> > > #Aristotle
> > >
> > >
> > >
> > > ------------------------------
> > >
> > > _______________________________________________
> > > Catalyst-dev mailing list
> > > Catalyst-dev at lists.rawmode.org
> > > http://lists.rawmode.org/mailman/listinfo/catalyst-dev
> > >
> > >
> > > End of Catalyst-dev Digest, Vol 23, Issue 2
> > > *******************************************

-- 
Alex Pavlovic - CTO
TF-1 Inc. ( Custom development, consultancy and training )
http://taskforce-1.com