From d3e00529333fb2eadac9c22a45f20c1050401952 Mon Sep 17 00:00:00 2001 From: Dylan William Hardison Date: Thu, 13 Sep 2018 22:37:55 +0200 Subject: Bug 1490708 - Ensure we always call DBIx::Connector->dbh before any DBI method (#744) The code didn't allow a way of doing this without a lot of work. So I had to take the following approach: The 'dbh' attribute is now a method that delegates to DBIx::Connector's dbh method. Per the docs, ->dbh() "Returns the connection's database handle. It will use a an existing handle if there is one, if the process has not been forked or a new thread spawned, and if the database is pingable. Otherwise, it will instantiate, cache, and return a new handle." Then there is the matter of the 'handles' on dbh. I've used Package::Stash to insert proxy methods into the class when it is loaded. --- Bugzilla/DB.pm | 43 ++++++++++++++++++++++++++++--------------- 1 file changed, 28 insertions(+), 15 deletions(-) (limited to 'Bugzilla/DB.pm') diff --git a/Bugzilla/DB.pm b/Bugzilla/DB.pm index 80404131a..cd5954219 100644 --- a/Bugzilla/DB.pm +++ b/Bugzilla/DB.pm @@ -14,15 +14,9 @@ use DBI; use DBIx::Connector; our %Connector; -has 'dbh' => ( +has 'connector' => ( is => 'lazy', - handles => [ - qw[ - begin_work column_info commit disconnect do errstr get_info last_insert_id ping prepare - primary_key quote_identifier rollback selectall_arrayref selectall_hashref - selectcol_arrayref selectrow_array selectrow_arrayref selectrow_hashref table_info - ] - ], + handles => [ qw( dbh ) ], ); use Bugzilla::Constants; @@ -44,6 +38,29 @@ has [qw(dsn user pass attrs)] => ( required => 1, ); + +# Install proxy methods to the DBI object. +# We can't use handles() as DBIx::Connector->dbh has to be called each +# time we need a DBI handle to ensure the connection is alive. +{ + my @DBI_METHODS = qw( + begin_work column_info commit disconnect do errstr get_info last_insert_id ping prepare + primary_key quote_identifier rollback selectall_arrayref selectall_hashref + selectcol_arrayref selectrow_array selectrow_arrayref selectrow_hashref table_info + ); + my $stash = Package::Stash->new(__PACKAGE__); + + foreach my $method (@DBI_METHODS) { + my $symbol = '&' . $method; + $stash->add_symbol( + $symbol => sub { + my $self = shift; + return $self->dbh->$method(@_); + } + ); + } +} + ##################################################################### # Constants ##################################################################### @@ -152,9 +169,7 @@ sub _connect { # instantiate the correct DB specific module - my $dbh = $pkg_module->new($params); - - return $dbh; + return $pkg_module->new($params); } sub _handle_error { @@ -1263,7 +1278,7 @@ sub bz_rollback_transaction { # Subclass Helpers ##################################################################### -sub _build_dbh { +sub _build_connector { my ($self) = @_; my ($dsn, $user, $pass, $override_attrs) = map { $self->$_ } qw(dsn user pass attrs); @@ -1295,9 +1310,7 @@ sub _build_dbh { } } - my $connector = $Connector{"$user.$dsn"} //= DBIx::Connector->new($dsn, $user, $pass, $attributes); - - return $connector->dbh; + return $Connector{"$user.$dsn"} //= DBIx::Connector->new($dsn, $user, $pass, $attributes); } ##################################################################### -- cgit v1.2.3-24-g4f1b From 8047b0395f5175da934f3aaf09b15a15d83da755 Mon Sep 17 00:00:00 2001 From: Dylan William Hardison Date: Mon, 24 Sep 2018 15:54:18 -0400 Subject: Bug 1492926 - Handle DBIx::Connectors more appropriately This is a bigger change than I anticipated, because the way we cached DBIx::Connector objects was bad. Now we cache the Bugzilla::DB instances in connect_main() and connect_shadow(). This is for maintaining a 1:1 mapping of Bugzilla::DB objects and DBIx::Connector objects. This is important because we want be able to inspect Bugzilla::DB->bz_in_transactions() from the 'connected' event. Note that we weaken the lexical variable $self in _build_connector() because it is referenced by the callback passed to DBI. Without this there would be a memory cycle and stuff would never be freed. (tested my understanding in this gist: https://gist.github.com/dylanwh/646574a027f7b7d92cb7586676da7468) --- Bugzilla/DB.pm | 41 ++++++++++++++++++++++++++++------------- 1 file changed, 28 insertions(+), 13 deletions(-) (limited to 'Bugzilla/DB.pm') diff --git a/Bugzilla/DB.pm b/Bugzilla/DB.pm index cd5954219..142c241bf 100644 --- a/Bugzilla/DB.pm +++ b/Bugzilla/DB.pm @@ -12,13 +12,13 @@ use Moo; use DBI; use DBIx::Connector; -our %Connector; has 'connector' => ( is => 'lazy', handles => [ qw( dbh ) ], ); +use Bugzilla::Logging; use Bugzilla::Constants; use Bugzilla::Install::Requirements; use Bugzilla::Install::Util qw(install_string); @@ -31,7 +31,9 @@ use Bugzilla::DB::Schema; use Bugzilla::Version; use List::Util qw(max); +use Scalar::Util qw(weaken); use Storable qw(dclone); +use English qw(-no_match_vars); has [qw(dsn user pass attrs)] => ( is => 'ro', @@ -44,7 +46,7 @@ has [qw(dsn user pass attrs)] => ( # time we need a DBI handle to ensure the connection is alive. { my @DBI_METHODS = qw( - begin_work column_info commit disconnect do errstr get_info last_insert_id ping prepare + begin_work column_info commit do errstr get_info last_insert_id ping prepare primary_key quote_identifier rollback selectall_arrayref selectall_hashref selectcol_arrayref selectrow_array selectrow_arrayref selectrow_hashref table_info ); @@ -130,6 +132,12 @@ sub quote { ##################################################################### sub connect_shadow { + state $shadow_dbh; + if ($shadow_dbh && $shadow_dbh->bz_in_transaction) { + FATAL("Somehow in a transaction at connection time"); + $shadow_dbh->bz_rollback_transaction(); + } + return $shadow_dbh if $shadow_dbh; my $params = Bugzilla->params; die "Tried to connect to non-existent shadowdb" unless Bugzilla->get_param_with_override('shadowdb'); @@ -147,13 +155,16 @@ sub connect_shadow { $connect_params->{db_user} = Bugzilla->localconfig->{'shadowdb_user'}; $connect_params->{db_pass} = Bugzilla->localconfig->{'shadowdb_pass'}; } - - return _connect($connect_params); + return $shadow_dbh = _connect($connect_params); } sub connect_main { - my $lc = Bugzilla->localconfig; - return _connect(Bugzilla->localconfig); + state $main_dbh = _connect(Bugzilla->localconfig); + if ($main_dbh->bz_in_transaction) { + FATAL("Somehow in a transaction at connection time"); + $main_dbh->bz_rollback_transaction(); + } + return $main_dbh; } sub _connect { @@ -293,7 +304,6 @@ sub _get_no_db_connection { my $dbh; my %connect_params = %{ Bugzilla->localconfig }; $connect_params{db_name} = ''; - local %Connector = (); my $conn_success = eval { $dbh = _connect(\%connect_params); }; @@ -1304,13 +1314,18 @@ sub _build_connector { } } my $class = ref $self; - if ($class->can('on_dbi_connected')) { - $attributes->{Callbacks} = { - connected => sub { $class->on_dbi_connected(@_); return }, - } - } + weaken($self); + $attributes->{Callbacks} = { + connected => sub { + my ($dbh, $dsn) = @_; + INFO("$PROGRAM_NAME connected mysql $dsn"); + ThrowCodeError('not_in_transaction') if $self && $self->bz_in_transaction; + $class->on_dbi_connected(@_) if $class->can('on_dbi_connected'); + return + }, + }; - return $Connector{"$user.$dsn"} //= DBIx::Connector->new($dsn, $user, $pass, $attributes); + return DBIx::Connector->new($dsn, $user, $pass, $attributes); } ##################################################################### -- cgit v1.2.3-24-g4f1b From 75dbfe1dc03748957f07eca5ac583bedc6fdba76 Mon Sep 17 00:00:00 2001 From: Dylan William Hardison Date: Tue, 9 Oct 2018 17:01:07 -0400 Subject: Bug 623384 - Use Module::Runtime instead of eval { require } or eval "use" --- Bugzilla/DB.pm | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'Bugzilla/DB.pm') diff --git a/Bugzilla/DB.pm b/Bugzilla/DB.pm index 142c241bf..87110aaaa 100644 --- a/Bugzilla/DB.pm +++ b/Bugzilla/DB.pm @@ -34,6 +34,7 @@ use List::Util qw(max); use Scalar::Util qw(weaken); use Storable qw(dclone); use English qw(-no_match_vars); +use Module::Runtime qw(require_module); has [qw(dsn user pass attrs)] => ( is => 'ro', @@ -174,7 +175,7 @@ sub _connect { my $pkg_module = DB_MODULE->{lc($driver)}->{db}; # do the actual import - eval ("require $pkg_module") + eval { require_module($pkg_module) } || die ("'$driver' is not a valid choice for \$db_driver in " . " localconfig: " . $@); -- cgit v1.2.3-24-g4f1b