Re: Connected Callback Errors Ignored
On Thu, Nov 14, 2013 at 02:07:41PM -0800, David E. Wheeler wrote:
On Nov 14, 2013, at 9:17 AM, David E. Wheeler da...@justatheory.com wrote:
The error about foo not existing is gone, overridden by the second
error about bar missing. This can lead to hard-to-find bugs. What
if the second query depended on a condition set up by the first, and
the first failed? As a user, I would only see the second error, and
think that the first statement had succeeded. It would take me quite
a while to figure out that it had not, in fact, succeeded.
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;
I’m also finding it means I can’t do error handling in the callback.
I'm not sure what you mean there.
I have to do this to get it to ignore errors:
$dbh-do('SET search_path = ?', undef, 'foo');
$dbh-set_err(undef) if $dbh-err;
I feel a little dirty.
Ignoring errors isn't a common action.
All of which is to say, your fix in 1.630 certainly improves the
situation, but since callbacks are really userland code, I think it
would be beneficial to change callbacks to run in an outer context,
with the outer DBI handles passed to them. Possible?
This would eliminate all of these problems, no?
Probably. There are three main down-sides to consider:
1. compatibility - the risk of breaking some existing callback code
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.
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.
Tim.
Re: Connected Callback Errors Ignored
On Nov 15, 2013, at 3:12 AM, Tim Bunce tim.bu...@pobox.com 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.
I’m also finding it means I can’t do error handling in the callback.
I'm not sure what you mean there.
I *always* use RaiseError = 1 (or HandleError). Never ever check return values.
I have to do this to get it to ignore errors:
$dbh-do('SET search_path = ?', undef, 'foo');
$dbh-set_err(undef) if $dbh-err;
I feel a little dirty.
Ignoring errors isn't a common action.
Oh, I agree, this is an edge case in that sense. The issue for me is more the
inconsistency. I have RaiseError set, but I cannot actually catch the error and
handle it where it happens, even though I have RaiseError set to true. I have
to know that callbacks are different, and either check return values to handle
errors (and call set_err(undef) if I have handled them in the callback), or
check return values and return (on 1.630 or later) to let an external error
handler catch the error. IOW, It’s a userland function but I have to write it
like an internal DBI function. It’s inconsistent UE.
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).
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?
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 can carve out a little time today to write some test cases if it’ll help.
Thanks,
David
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 tim.bu...@pobox.com 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.
Re: Connected Callback Errors Ignored
On Nov 15, 2013, at 10:24 AM, Tim Bunce tim.bu...@pobox.com wrote: When the method call (which fired the callback) returns, the error recorded on the handle will trigger RaiseError etc. Only on 1.630 and higher. I *always* use RaiseError = 1 (or HandleError). Never ever check return values. Ah, your use of HandleError adds an extra perspective on this. Yeah, I have been defaulting to it for so long that I forget that some folks might not. 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. And only on connection or reconnection, yes? Not like for every call to do() or fetch()! 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 :) Cool. Not pushed yet, I guess. Will watch for it! Thanks! David
Re: Connected Callback Errors Ignored
On Wed, Nov 13, 2013 at 11:22:03AM -0800, David E. Wheeler wrote:
DBIers,
Given this script:
use v5.18;
use warnings;
use utf8;
use DBI;
my $dbh = DBI-connect('dbi:SQLite:', '', '', {
PrintError = 0,
RaiseError = 1,
AutoCommit = 1,
Callbacks = {
connected = sub {
say 'connected';
$_[0]-do('SELECT 1 from foo');
return;
},
}
});
say 'go';
$dbh-do('SELECT 1 from foo');
The output is:
connected
go
DBD::SQLite::db do failed: no such table: foo at /Users/david/bin/try
line 22.
That doesn't seem right to me. Shouldn't the connected callback throw an
exception? IOW, I would not expect go to be output.
This turned out to be Quite Interesting
Firstly, the RaiseError/PrintError logic only applies to 'top level
calls', i.e. calls made by the application. That's why code executed
'within' the DBI and drivers has to do it's own error checking.
Another issue is that the callback is passed the 'inner' handle not the
outer (tied) one. That's not documented but ought to be as it's bound to
lead to surprises.
So in this example the do() call isn't passing through the DBI's method
dispatcher. It's calling the drivers own do() method directly. That's
not a big deal in this case as the dispatcher wouldn't have applied the
RaiseError logic anyway as the call was made from 'within' the DBI.
However, the do() does record an error on the handle which should be
noticed by the DBI dispatcher when the connect() method is returning.
The problem here seems to be that the callback is on the connected()
method and that's marked as 'keep err'. [...later...]
It turns out that this is fixed by the changes I made in 1.630 to
improve the handling of errors recorded in methods that are marked
'keep err'. That change was originally for STORE but also fixes this.
So I'll take a guess that you're not using DBI 1.630. (And hope I'm right.)
Tim.
Re: Connected Callback Errors Ignored
On Nov 14, 2013, at 3:47 AM, Tim Bunce tim.bu...@pobox.com wrote: So I'll take a guess that you're not using DBI 1.630. (And hope I'm right.) Yep, I am using 1.628. I'm wondering, though, if callbacks should not be considered internal calls, and the outer handle passed to them? D
Re: Connected Callback Errors Ignored
On Nov 14, 2013, at 9:17 AM, David E. Wheeler da...@justatheory.com wrote:
The error about foo not existing is gone, overridden by the second error
about bar missing. This can lead to hard-to-find bugs. What if the second
query depended on a condition set up by the first, and the first failed? As a
user, I would only see the second error, and think that the first statement
had succeeded. It would take me quite a while to figure out that it had not,
in fact, succeeded.
I’m also finding it means I can’t do error handling in the callback. I have to
do this to get it to ignore errors:
$dbh-do('SET search_path = ?', undef, 'foo');
$dbh-set_err(undef) if $dbh-err;
I feel a little dirty.
All of which is to say, your fix in 1.630 certainly improves the situation,
but since callbacks are really userland code, I think it would be beneficial
to change callbacks to run in an outer context, with the outer DBI handles
passed to them. Possible?
This would eliminate all of these problems, no?
Thanks,
David
Connected Callback Errors Ignored
DBIers,
Given this script:
use v5.18;
use warnings;
use utf8;
use DBI;
my $dbh = DBI-connect('dbi:SQLite:', '', '', {
PrintError = 0,
RaiseError = 1,
AutoCommit = 1,
Callbacks = {
connected = sub {
say 'connected';
$_[0]-do('SELECT 1 from foo');
return;
},
}
});
say 'go';
$dbh-do('SELECT 1 from foo');
The output is:
connected
go
DBD::SQLite::db do failed: no such table: foo at /Users/david/bin/try line
22.
That doesn't seem right to me. Shouldn't the connected callback throw an
exception? IOW, I would not expect go to be output.
Best,
David
