Protect db_handle methods against "MySQL server has gone away"
Created by: muffato
Use case
The issue was raised by @MatBarba . In ENSCORESW-2913 he managed to identify that a Python Runnable that just sleeps would raise the following error after 30 min:
DBD::mysql::db primary_key failed: MySQL server has gone away at (...)/modules/Bio/EnsEMBL/Hive/DBSQL/BaseAdaptor.pm line 244, <$CHILD_RDR> line 6.
30 min is indeed the timeout parameter of the server he was using.
Description of the changes
The primary_key
"dbh" method (and a few others) were indeed called on database handles without checking if the handle is still valid, and without any eval {}
protection, even though such protection is available via the AUTOLOAD
in DBSQL::DBConnection
. The fix is to move those calls from the database handle to the database connection (new "best practice").
prepare
is slightly more tricky to fix since there is already a prepare
in DBConnection
. In this case, I have introduced a fake DBConnection method named protected_prepare
that calls prepare
on the database handle with the "MySQL server has gone away" protection, but without disconnecting from the database even if disconnect_when_inactive
is set. Indeed, prepare
is most often called on SELECT statements, which are immediately followed by execute
and fetch
, both of which expect the database connection to still be open
Comments
I have added a few comments about pitfalls of the current implementation
Notes
The change in has_write_access
will probably clash with the changes that have been accepted upstream. I can easily fix it.
Merge request reports
Activity
Created by: codecov[bot]
Codecov Report
Merging #58 into version/2.5 will increase coverage by
0.24%
. The diff coverage is93.33%
.@@ Coverage Diff @@ ## version/2.5 #58 +/- ## =============================================== + Coverage 80.73% 80.97% +0.24% =============================================== Files 170 170 Lines 9888 9898 +10 Branches 1597 1598 +1 =============================================== + Hits 7983 8015 +32 + Misses 1248 1230 -18 + Partials 657 653 -4
Impacted Files Coverage Δ modules/Bio/EnsEMBL/Hive/DBSQL/BaseAdaptor.pm 83.33% <100%> (-0.06%)
modules/Bio/EnsEMBL/Hive/DBSQL/DBConnection.pm 57.05% <100%> (ø)
modules/Bio/EnsEMBL/Hive/DBSQL/CoreDBConnection.pm 53.41% <100%> (+6.04%)
t/02.api/latest_schema_patch.t 94.91% <100%> (ø)
modules/Bio/EnsEMBL/Hive/DBSQL/StatementHandle.pm 76.54% <100%> (ø)
t/02.api/drop_hive_tables.t 94.73% <100%> (ø)
t/02.api/reconnect.t 89.79% <88.88%> (+2.29%)
modules/Bio/EnsEMBL/Hive/Queen.pm 69.7% <0%> (+0.26%)
modules/Bio/EnsEMBL/Hive/Utils/Test.pm 82.51% <0%> (+0.54%)
modules/Bio/EnsEMBL/Hive/Worker.pm 65.91% <0%> (+0.56%)
... and 2 more
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update e6611fa...8f5caed. Read the comment docs.Created by: muffato
Ah, good point though. I didn't think of that. Here is a summary of how the different bits are connected.
-
DBConnection
hasprotected_prepare_execute
which runsprepare
+execute
and protects primarily against deadlocks and lock timeouts, but also inherits the protection described below -
prepare
will create a newStatementHandle
with the lower-levelprotected_prepare
-
CoreDBConnection
(DBConnection
's parent class) hasprotected_prepare
viaAUTOLOAD
. The latter is used to call dbh methods directly on the DBConnection, catching "MySQL server has gone away errors" -
StatementHandle
also has anAUTOLOAD
to pass all its method calls to the underlying sth, also catching "MySQL server has gone away errors"
I may add that to the developer documentation ?
-