Fix missing handling of on_(dis)connect* failures
authorPeter Rabbitson <ribasushi@cpan.org>
Fri, 3 Apr 2015 22:18:39 +0000 (00:18 +0200)
committerPeter Rabbitson <ribasushi@cpan.org>
Sat, 4 Apr 2015 10:45:56 +0000 (12:45 +0200)
commit534521dac62f6ab58e83a42d4e8e3cb586db464b
tree8c6242a3f995658162ec367f064910572d3c7ef4
parenta4f3b65c5eeb1c1fbe41128dc97b88e1b7f506e5
Fix missing handling of on_(dis)connect* failures

Time for a short story (see last paragraph for TL;DR): Some hours ago an
innocent question was asked in the #dbix-class channel, apparently
related to a mysterious failure to determine the RDBMS version. Before I
was able to properly investigate the problem, someone else piped up with
"Oh I've seen this before, and I worked around it". That was of course
without telling anyone else </headdesk>

After a little back and forth it became apparent that if the on_connect
settings could not be executed for whatever reason, the result may be an
implicitly failed connection attempt which is *entirely undetected* as
it leaves the original $dbh in place with some or all of the on_connect
instructions not having executed as expected.

To reiterate: if one manages to:
 * supply a malformed on_connect*
and then
 * call as the first order of business some codepath that can mask away
   the connection failure (say ->deploy under sqlite)
then the result is a $schema with a proper $dbh in the storage instance,
but *without* some (or all) of the on_connect instructions having been
executed. As a bonus the actual call to get e.g. the RDBMS metadata (like
version etc) will have failed completely silently as well. Arguably a
rather problematic bug.

The above and by extension an issue with silent on_disconnect* failures
are bith fixed by this commit. This was possible *only* because the
second person came into the channel and reported it, instead of silently
fixing things and moving on to whatever they were doing.

Moreover while preparing the (relatively modest) patch to fix this issue,
a small refactor revealed a pretty serious bug in the XS accessor provider
Class::XSAccessor: https://rt.cpan.org/Ticket/Display.html?id=103296
Yes, it took about 3 hours to diagnose this and hunt down an isolated
failcase, but the bug severety (and its elegance) are totally worth it.

Moral of the story: *PLEASE* report issues upstream. Even if you figured
out a quick workaround, the devs still need to know about the problem you
did encounter in the first place. Bugs that nobody knows about can *NOT*
be fixed, and our software commons can not improve. So *PLEASE* speak up.
Changes
lib/DBIx/Class/Storage/DBI.pm
t/storage/error.t