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>
Fri, 17 Jun 2016 13:41:14 +0000 (15:41 +0200)
commit0f69bb8ba3069d88015161ddd8afbf793bf50c28
tree4fcb3d8ddfe6c803dca5188bcb312721c11a1672
parent294c6f29842319f3f8ddcd215d770ea980d6a494
Fix missing handling of on_(dis)connect* failures

( cherry-pick of b81538923 )

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