From: Peter Rabbitson <ribasushi@cpan.org>
Date: Tue, 4 Mar 2014 08:39:49 +0000 (+0100)
Subject: Fix pesky on_connect_* race condition abraxxa++ ilmari++
X-Git-Tag: v0.082800~232
X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=1eb87dd767c4bdb815085acb2a8e63e12b32f990;p=dbsrgits%2FDBIx-Class.git

Fix pesky on_connect_* race condition abraxxa++ ilmari++

A race condition existed between storage accessor setters and the
determine_driver routines, triggering a connection before the set-cycle
is finished
---

diff --git a/Changes b/Changes
index d557c39..387cb0a 100644
--- a/Changes
+++ b/Changes
@@ -1,5 +1,10 @@
 Revision history for DBIx::Class
 
+    * Fixes
+        - Fix on_connect_* not always firing in some cases - a race condition
+          existed between storage accessor setters and the determine_driver
+          routines, triggering a connection before the set-cycle is finished
+
 0.08270 2014-01-30 21:54 (PST)
     * Fixes
         - Fix 0.08260 regression in DBD::SQLite bound int handling. Inserted
diff --git a/lib/DBIx/Class/Storage/DBI.pm b/lib/DBIx/Class/Storage/DBI.pm
index 23a7f71..9b5e3b0 100644
--- a/lib/DBIx/Class/Storage/DBI.pm
+++ b/lib/DBIx/Class/Storage/DBI.pm
@@ -79,20 +79,24 @@ __PACKAGE__->_use_join_optimizer (1);
 sub _determine_supports_join_optimizer { 1 };
 
 # Each of these methods need _determine_driver called before itself
-# in order to function reliably. This is a purely DRY optimization
+# in order to function reliably. We also need to separate accessors
+# from plain old method calls, since an accessor called as a setter
+# does *not* need the driver determination loop fired (and in fact
+# can produce hard to find bugs, like e.g. losing on_connect_*
+# semantics on fresh connections)
 #
-# get_(use)_dbms_capability need to be called on the correct Storage
-# class, as _use_X may be hardcoded class-wide, and _supports_X calls
-# _determine_supports_X which obv. needs a correct driver as well
-my @rdbms_specific_methods = qw/
+# The construct below is simply a parameterized around()
+my $storage_accessor_idx = { map { $_ => 1 } qw(
   sqlt_type
-  deployment_statements
+  datetime_parser_type
 
   sql_maker
   cursor_class
+)};
+for my $meth (keys %$storage_accessor_idx, qw(
+  deployment_statements
 
   build_datetime_parser
-  datetime_parser_type
 
   txn_begin
 
@@ -110,15 +114,13 @@ my @rdbms_specific_methods = qw/
 
   _server_info
   _get_server_version
-/;
-
-for my $meth (@rdbms_specific_methods) {
+)) {
 
   my $orig = __PACKAGE__->can ($meth)
     or die "$meth is not a ::Storage::DBI method!";
 
-  no strict qw/refs/;
-  no warnings qw/redefine/;
+  no strict 'refs';
+  no warnings 'redefine';
   *{__PACKAGE__ ."::$meth"} = subname $meth => sub {
     if (
       # only fire when invoked on an instance, a valid class-based invocation
@@ -129,6 +131,10 @@ for my $meth (@rdbms_specific_methods) {
         and
       ! $_[0]->{_in_determine_driver}
         and
+      # if this is a known *setter* - just set it, no need to connect
+      # and determine the driver
+      ! ( $storage_accessor_idx->{$meth} and @_ > 1 )
+        and
       # Only try to determine stuff if we have *something* that either is or can
       # provide a DSN. Allows for bare $schema's generated with a plain ->connect()
       # to still be marginally useful
diff --git a/t/storage/on_connect_do.t b/t/storage/on_connect_do.t
index 2874a9d..115fadb 100644
--- a/t/storage/on_connect_do.t
+++ b/t/storage/on_connect_do.t
@@ -36,6 +36,11 @@ $schema->storage->disconnect;
 ok $schema->connection(
     sub { DBI->connect(DBICTest->_database, undef, undef, { AutoCommit => 0 }) },
     {
+        # DO NOT REMOVE - this seems like an unrelated piece of info,
+        # but is in fact a test for a bug where setting an accessor-via-option
+        # would trigger an early connect *bypassing* the on_connect_* pieces
+        cursor_class => 'DBIx::Class::Storage::Cursor',
+
         on_connect_do       => [
             'CREATE TABLE TEST_empty (id INTEGER)',
             [ 'INSERT INTO TEST_empty VALUES (?)', {}, 2 ],