Skip Menu |

This queue is for tickets about the DBI CPAN distribution.

Report information
The Basics
Id: 64330
Status: resolved
Priority: 0/
Queue: DBI

People
Owner: Nobody in particular
Requestors: VLYON [...] cpan.org
Cc:
AdminCc:

Bug Information
Severity: Normal
Broken in: 1.611
Fixed in: (no value)



Subject: ping() wipes out the errstr
The ping() method removes an existing error. I've tested this using Sponge and SQLite, which don't provide their own ping() methods, so that means the bug lies in the DBI. Here's how to reproduce: my $dbh = DBI->connect('DBI:Sponge:') or die $DBI::errstr; $dbh->do('abc'); # This will raise an error. warn $dbh->errstr; # prints the error $dbh->ping; warn $dbh->errstr; # now errstr is undefined! According to the DBI docs ping() should NOT reset the err. http://search.cpan.org/perldoc?DBI#err
On Thu Dec 30 06:40:17 2010, VLYON wrote: Show quoted text
> The ping() method removes an existing error. > I've tested this using Sponge and SQLite, which don't provide their own > ping() methods, so that means the bug lies in the DBI. > > Here's how to reproduce: > > my $dbh = DBI->connect('DBI:Sponge:') or die $DBI::errstr; > $dbh->do('abc'); # This will raise an error. > warn $dbh->errstr; # prints the error > $dbh->ping; > warn $dbh->errstr; # now errstr is undefined! > > > According to the DBI docs ping() should NOT reset the err. > http://search.cpan.org/perldoc?DBI#err
Tests added to subversion trunk to demonstrate this problem. Martin -- Martin J. Evans Wetherby, UK
On Tue Mar 15 15:21:02 2011, MJEVANS wrote: Show quoted text
> Tests added to subversion trunk to demonstrate this problem.
Thanks Martin, but which tests and in what svn revision? I don't see any failing tests for this. Was it fixed? I don't see a note in the Changes file. Tim.
Nevermind. I've spotted the (now passing) TODO test in t/08keeperr.t. Thanks. Closed.
ping() still wipes out the errstr when a driver doesn't have its own ping (). Attached a test patch. Regards.
Subject: ping_test.diff
Index: t/08keeperr.t =================================================================== --- t/08keeperr.t (ƒŠƒrƒWƒ‡ƒ“ 15377) +++ t/08keeperr.t (ì‹ÆƒRƒs[) @@ -2,7 +2,7 @@ use strict; -use Test::More tests => 79; +use Test::More tests => 89; ## ---------------------------------------------------------------------------- ## 08keeperr.t @@ -71,6 +71,7 @@ # test ping does not destroy the errstr sub ping_keeps_err { + # for a driver which has its own ping my $dbh = DBI->connect('DBI:ExampleP:', undef, undef, { PrintError => 0 }); $dbh->set_err(42, "ERROR 42"); @@ -90,6 +91,29 @@ # so here we just test that there is an error ok $dbh->err, "err true after failed ping"; ok $dbh->errstr, "errstr true after failed ping"; + + # for a driver which doesn't have its own ping + $dbh = DBI->connect('DBI:Sponge:', undef, undef, { PrintError => 0 }); + $dbh->STORE(Active => 1); + + $dbh->set_err(42, "ERROR 42"); + is $dbh->err, 42; + is $dbh->errstr, "ERROR 42"; + ok $dbh->ping, "ping returns true: ".$dbh->ping; + is $dbh->err, 42, "err unchanged after ping"; + is $dbh->errstr, "ERROR 42", "errstr unchanged after ping"; + + $dbh->disconnect; + $dbh->STORE(Active => 0); + + $dbh->set_err(42, "ERROR 42"); + is $dbh->err, 42, "err unchanged after ping"; + is $dbh->errstr, "ERROR 42", "errstr unchanged after ping"; + ok !$dbh->ping, "ping returns false"; + # it's reasonable for ping() to set err/errstr if it fails + # so here we just test that there is an error + ok $dbh->err, "err true after failed ping"; + ok $dbh->errstr, "errstr true after failed ping"; } ## ----------------------------------------------------------------------------
On Thu Sep 06 10:32:27 2012, ISHIGAKI wrote: Show quoted text
> ping() still wipes out the errstr when a driver doesn't have its own
ping Show quoted text
> (). Attached a test patch. Regards. > > >
That is probably because when you call ping and the driver does not implement it DBI calls _not_impl and it does not have IMA_KEEP_ERR set. Obviously adding IMA_KEEP_ERR to _not_impl will fix it and as far as I can see _not_impl is only used in one place but I'm not sure what Tim intended. It could be an easy fix; it might have other consequences I'm not ware of. Martin -- Martin J. Evans Wetherby, UK
On Fri Sep 07 08:19:22 2012, MJEVANS wrote: Show quoted text
> On Thu Sep 06 10:32:27 2012, ISHIGAKI wrote:
> > ping() still wipes out the errstr when a driver doesn't have its own
> ping
> > (). Attached a test patch. Regards. > > > > > >
> > That is probably because when you call ping and the driver does not > implement it DBI calls _not_impl and it does not have IMA_KEEP_ERR
set. Show quoted text
> Obviously adding IMA_KEEP_ERR to _not_impl will fix it and as far as I > can see _not_impl is only used in one place but I'm not sure what Tim > intended. It could be an easy fix; it might have other consequences
I'm Show quoted text
> not ware of. > > Martin
Try this patch (applied to current subversion trunk): Index: DBI.pm =================================================================== --- DBI.pm (revision 15379) +++ DBI.pm (working copy) @@ -390,7 +390,6 @@ 'FIRSTKEY' => $keeperr, 'NEXTKEY' => $keeperr, 'STORE' => { O=>0x0418 | 0x4 }, - _not_impl => undef, can => { O=>0x0100 }, # special case, see dispatch debug => { U =>[1,2,'[$debug_level]'], O=>0x0004 }, # old name for trace dump_handle => { U =>[1,3,'[$message [, $level]]'], O=>0x0004 }, @@ -1344,12 +1343,6 @@ # methods common to all handle types: - sub _not_impl { - my ($h, $method) = @_; - $h->trace_msg("Driver does not implement the $method method.\n"); - return; # empty list / undef - } - # generic TIEHASH default methods: sub FIRSTKEY { } sub NEXTKEY { } @@ -1711,7 +1704,6 @@ sub ping { my $dbh = shift; - $dbh->_not_impl('ping'); # "0 but true" is a special kind of true 0 that is used here so # applications can check if the ping was a real ping or not ($dbh->FETCH('Active')) ? "0 but true" : 0; -- Martin J. Evans Wetherby, UK
On Thu Sep 06 10:32:27 2012, ISHIGAKI wrote: Show quoted text
> ping() still wipes out the errstr when a driver doesn't have its own ping > (). Attached a test patch. Regards. > > >
In your patch you're setting $dbh->STORE(Active => ?) when you connect and disconnect. This should not be needed. The DBI should be doing this for the 'Sponge' driver. (Another bug?) I'd recommend the attached patch, but this results in a few failed tests: Test Summary Report ------------------- t/08keeperr.t (Wstat: 256 Tests: 89 Failed: 1) Failed test: 82 Non-zero exit status: 1 t/zvg_08keeperr.t (Wstat: 512 Tests: 89 Failed: 2) Failed tests: 79, 82 Non-zero exit status: 2 t/zvp_08keeperr.t (Wstat: 256 Tests: 89 Failed: 1) Failed test: 87 Non-zero exit status: 1 t/zvxgp_08keeperr.t (Wstat: 512 Tests: 89 Failed: 2) Failed tests: 79, 87 Non-zero exit status: 2 Files=183, Tests=8870, 114 wallclock secs ( 0.86 usr 2.45 sys + 16.55 cusr 80.54 csys = 100.40 CPU) Result: FAIL Failed 4/183 test programs. 6/8870 subtests failed.
Subject: patch.diff
Index: TODO_2005.txt =================================================================== --- TODO_2005.txt (revision 15382) +++ TODO_2005.txt (working copy) @@ -142,7 +142,7 @@ pre and post call hooks via ima structure? -Remove _not_impl. Alias debug to trace in DBI::(dr/db/st) and remove +Alias debug to trace in DBI::(dr/db/st) and remove debug() method from internals. DBD::Multiplex enhancements (Thomas Kishel <tom@kishel.net>): Index: t/08keeperr.t =================================================================== --- t/08keeperr.t (revision 15382) +++ t/08keeperr.t (working copy) @@ -2,7 +2,7 @@ use strict; -use Test::More tests => 79; +use Test::More tests => 89; ## ---------------------------------------------------------------------------- ## 08keeperr.t @@ -71,11 +71,12 @@ # test ping does not destroy the errstr sub ping_keeps_err { + # for a driver which has its own ping my $dbh = DBI->connect('DBI:ExampleP:', undef, undef, { PrintError => 0 }); $dbh->set_err(42, "ERROR 42"); - is $dbh->err, 42; - is $dbh->errstr, "ERROR 42"; + is $dbh->err, 42, "setup err"; + is $dbh->errstr, "ERROR 42", "setup errstr"; ok $dbh->ping, "ping returns true"; is $dbh->err, 42, "err unchanged after ping"; is $dbh->errstr, "ERROR 42", "errstr unchanged after ping"; @@ -83,13 +84,30 @@ $dbh->disconnect; $dbh->set_err(42, "ERROR 42"); + is $dbh->err, 42, "setup err"; + is $dbh->errstr, "ERROR 42", "setup errstr"; + ok !$dbh->ping, "ping returns false"; is $dbh->err, 42, "err unchanged after ping"; is $dbh->errstr, "ERROR 42", "errstr unchanged after ping"; + + # for a driver which doesn't have its own ping + $dbh = DBI->connect('DBI:Sponge:', undef, undef, { PrintError => 0 }); + + $dbh->set_err(42, "ERROR 42"); + is $dbh->err, 42, "setup err"; + is $dbh->errstr, "ERROR 42", "setup errstr"; + ok $dbh->ping, "ping returns true"; + is $dbh->err, 42, "err unchanged after ping"; + is $dbh->errstr, "ERROR 42", "errstr unchanged after ping"; + + $dbh->disconnect; + + $dbh->set_err(42, "ERROR 42"); + is $dbh->err, 42, "setup err"; + is $dbh->errstr, "ERROR 42", "setup errstr"; ok !$dbh->ping, "ping returns false"; - # it's reasonable for ping() to set err/errstr if it fails - # so here we just test that there is an error - ok $dbh->err, "err true after failed ping"; - ok $dbh->errstr, "errstr true after failed ping"; + is $dbh->err, 42, "err unchanged after ping"; + is $dbh->errstr, "ERROR 42", "errstr unchanged after ping"; } ## ---------------------------------------------------------------------------- Index: DBI.pm =================================================================== --- DBI.pm (revision 15382) +++ DBI.pm (working copy) @@ -390,7 +390,6 @@ 'FIRSTKEY' => $keeperr, 'NEXTKEY' => $keeperr, 'STORE' => { O=>0x0418 | 0x4 }, - _not_impl => undef, can => { O=>0x0100 }, # special case, see dispatch debug => { U =>[1,2,'[$debug_level]'], O=>0x0004 }, # old name for trace dump_handle => { U =>[1,3,'[$message [, $level]]'], O=>0x0004 }, @@ -1344,12 +1343,6 @@ # methods common to all handle types: - sub _not_impl { - my ($h, $method) = @_; - $h->trace_msg("Driver does not implement the $method method.\n"); - return; # empty list / undef - } - # generic TIEHASH default methods: sub FIRSTKEY { } sub NEXTKEY { } @@ -1711,7 +1704,6 @@ sub ping { my $dbh = shift; - $dbh->_not_impl('ping'); # "0 but true" is a special kind of true 0 that is used here so # applications can check if the ping was a real ping or not ($dbh->FETCH('Active')) ? "0 but true" : 0;
Should be fixed in subversion trunk now - basically, I added the patch I first posted in this RT. Also added your new test cases. Thanks Martin -- Martin J. Evans Wetherby, UK
Can we close this one now?
On Sun Sep 21 08:51:39 2014, TIMB wrote: Show quoted text
> Can we close this one now?
As far as I am concerned, yes. But I didn't report it originally. Martin -- Martin J. Evans Wetherby, UK
I'll have a quick look this afternoon, then I'll resolve it if pos. -- Vernon