Re: Connected Callback Errors Ignored

On Fri, Nov 15, 2013 at 09:14:58AM -0800, David E. Wheeler wrote:
> On Nov 15, 2013, at 3:12 AM, Tim Bunce <[email protected]> wrote:
> 
> > You'd need to write the callback code in the way you'd naturally write
> > it when not using RaiseError. Which typically means something like:
> > 
> >    $dbh->do('SET search_path = ?', undef, 'foo')
> >        or return;
> 
> That will prevent any further code from executing, but does not give me a chance to handle the error.

When the method call (which fired the callback) returns, the error
recorded on the handle will trigger RaiseError etc.

> I *always* use RaiseError => 1 (or HandleError). Never ever check return values.

Ah, your use of HandleError adds an extra perspective on this.

> > Probably. There are three main down-sides to consider:
> > 1. compatibility - the risk of breaking some existing callback code
> 
> Seems unlikely, I think, since it’s not documented that it’s not the
> user’s handle that is passed to callbacks. I think I have been using
> callbacks longer than anyone and never noticed. Frankly I’m amazed at
> how few people know they’re there (I regularly get requests to add
> them to DBIx::Connector).

I agree (on all points :).

> > 2. performance - the cost of getting the outer handle
> > 3. pure-perl compatibility
> > 
> > There is an undocumented DBI::_handles function that returns the inner
> > and outer handles for a given (inner or outer) handle. Perl code within
> > DBI.pm, such as the connect_cached callback logic, would need to call
> > that (or something like it) to get the outer handle to pass to the callback.
> 
> Yeah. That’s not a ton over overhead, is it? Is an additional method
> call really likely to have much of a performance impact?

Probably not. I think connect_cached is the only use of Callbacks in
DBI.pm (not DBI.xs), and the very small added cost would only be paid by
those using Callbacks anyway.

> > The DBI::_handles implementation in lib/DBI/PurePerl.pm doesn't support
> > getting the outer handle from an inner one. That's probably fixable by
> > adding a weakref.
> > 
> > I don't have a strong opinion on this.
> 
> I am strongly in favor of providing a consistent interface to users.
> Currently, error-handling in callbacks is inconsistent when using
> RaiseError or HandleError.

I agree.

> I can carve out a little time today to write some test cases if it’ll help.

No need, I've just hacked t/70callbacks.t to include those tests :)

Tim.