diff --git a/lib/dml/moodle_read_slave_trait.php b/lib/dml/moodle_read_slave_trait.php index cba970ed624..40478df545f 100644 --- a/lib/dml/moodle_read_slave_trait.php +++ b/lib/dml/moodle_read_slave_trait.php @@ -326,6 +326,15 @@ trait moodle_read_slave_trait { if (isset($this->written[$tablename])) { if ($this->slavelatency) { $now = $now ?: microtime(true); + // Paranoid check. + if ($this->written[$tablename] === true) { + debugging( + "$tablename last written set to true outside transaction - should not happen!", + DEBUG_DEVELOPER + ); + $this->written[$tablename] = $now; + return false; + } if ($now - $this->written[$tablename] < $this->slavelatency) { return false; } @@ -339,7 +348,6 @@ trait moodle_read_slave_trait { return true; case SQL_QUERY_INSERT: case SQL_QUERY_UPDATE: - // If we are in transaction we cannot set the written time yet. $now = $this->slavelatency && !$this->transactions ? microtime(true) : true; foreach ($this->table_names($sql) as $tablename) { $this->written[$tablename] = $now; @@ -364,23 +372,15 @@ trait moodle_read_slave_trait { * @throws dml_transaction_exception Creates and throws transaction related exceptions. */ public function commit_delegated_transaction(moodle_transaction $transaction) { - parent::commit_delegated_transaction($transaction); - - if ($this->transactions) { - return; - } - - if (!$this->slavelatency) { - return; - } - - $now = null; - foreach ($this->written as $tablename => $when) { - if ($when === true) { - $now = $now ?: microtime(true); + if ($this->slavelatency && $this->written) { + // Adjust the written time. + $now = microtime(true); + foreach ($this->written as $tablename => $when) { $this->written[$tablename] = $now; } } + + parent::commit_delegated_transaction($transaction); } /** diff --git a/lib/dml/tests/dml_read_slave_test.php b/lib/dml/tests/dml_read_slave_test.php index 4ee6fc9c620..2193849e92a 100644 --- a/lib/dml/tests/dml_read_slave_test.php +++ b/lib/dml/tests/dml_read_slave_test.php @@ -21,12 +21,14 @@ * @category dml * @copyright 2018 Srdjan Janković, Catalyst IT * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @coversDefaultClass \moodle_temptables */ defined('MOODLE_INTERNAL') || die(); require_once(__DIR__.'/fixtures/read_slave_moodle_database_table_names.php'); require_once(__DIR__.'/fixtures/read_slave_moodle_database_special.php'); +require_once(__DIR__.'/../../tests/fixtures/event_fixtures.php'); /** * DML read/read-write database handle use tests @@ -36,7 +38,7 @@ require_once(__DIR__.'/fixtures/read_slave_moodle_database_special.php'); * @copyright 2018 Catalyst IT * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ -class core_dml_read_slave_testcase extends base_testcase { +class dml_read_slave_test extends \base_testcase { /** @var float */ static private $dbreadonlylatency = 0.8; @@ -278,12 +280,15 @@ class core_dml_read_slave_testcase extends base_testcase { * so the latency parameter is applied properly. * * @return void + * @covers ::can_use_readonly + * @covers ::commit_delegated_transaction */ - public function test_transaction() : void { + public function test_transaction(): void { $DB = $this->new_db(true); $this->assertNull($DB->get_dbhwrite()); + $skip = false; $transaction = $DB->start_delegated_transaction(); $now = microtime(true); $handle = $DB->get_records_sql("SELECT * FROM {table}"); @@ -304,11 +309,92 @@ class core_dml_read_slave_testcase extends base_testcase { // Make sure enough time passes. sleep(1); + } else { + $skip = true; } // Exceeded latency time, use ro handle. $handle = $DB->get_records_sql("SELECT * FROM {table}"); $this->assert_readonly_handle($handle); + + if ($skip) { + $this->markTestSkipped("Delay too long to test write handle immediately after transaction"); + } + } + + /** + * Test readonly handle is not used with events + * when the latency parameter is applied properly. + * + * @return void + * @covers ::can_use_readonly + * @covers ::commit_delegated_transaction + */ + public function test_transaction_with_events(): void { + $this->with_global_db(function () { + global $DB; + + $DB = $this->new_db(true, ['test_ro'], read_slave_moodle_database_special::class); + $DB->set_tables([ + 'config_plugins' => [ + 'columns' => [ + 'plugin' => (object)['meta_type' => ''], + ] + ] + ]); + + $this->assertNull($DB->get_dbhwrite()); + + $this->_called = false; + $transaction = $DB->start_delegated_transaction(); + $now = microtime(true); + + $observers = [ + [ + 'eventname' => '\core_tests\event\unittest_executed', + 'callback' => function (\core_tests\event\unittest_executed $event) use ($DB, $now) { + $this->_called = true; + $this->assertFalse($DB->is_transaction_started()); + + // This condition should always evaluate true, however we need to + // safeguard from an unaccounted delay that can break this test. + if (microtime(true) - $now < 1 + self::$dbreadonlylatency) { + // Not enough time passed, use rw handle. + $handle = $DB->get_records_sql_p("SELECT * FROM {table}"); + $this->assertEquals('test_rw::test:test', $handle); + + // Make sure enough time passes. + sleep(1); + } else { + $this->markTestSkipped("Delay too long to test write handle immediately after transaction"); + } + + // Exceeded latency time, use ro handle. + $handle = $DB->get_records_sql_p("SELECT * FROM {table}"); + $this->assertEquals('test_ro::test:test', $handle); + }, + 'internal' => 0, + ], + ]; + \core\event\manager::phpunit_replace_observers($observers); + + $handle = $DB->get_records_sql_p("SELECT * FROM {table}"); + // Use rw handle during transaction. + $this->assertEquals('test_rw::test:test', $handle); + + $handle = $DB->insert_record_raw('table', array('name' => 'blah')); + // Introduce delay so we can check that table write timestamps + // are adjusted properly. + sleep(1); + $event = \core_tests\event\unittest_executed::create([ + 'context' => \context_system::instance(), + 'other' => ['sample' => 1] + ]); + $event->trigger(); + $transaction->allow_commit(); + + $this->assertTrue($this->_called); + }); } /** diff --git a/lib/dml/tests/fixtures/read_slave_moodle_database_special.php b/lib/dml/tests/fixtures/read_slave_moodle_database_special.php index f34491935ee..c2f98d087a1 100644 --- a/lib/dml/tests/fixtures/read_slave_moodle_database_special.php +++ b/lib/dml/tests/fixtures/read_slave_moodle_database_special.php @@ -26,7 +26,6 @@ defined('MOODLE_INTERNAL') || die(); require_once(__DIR__.'/read_slave_moodle_database.php'); -require_once(__DIR__.'/read_slave_moodle_recordset_special.php'); /** * Database driver mock test class that uses read_slave_moodle_recordset_special @@ -50,6 +49,19 @@ class read_slave_moodle_database_special extends read_slave_moodle_database { return []; } + /** + * Returns read_slave_moodle_database::get_records_sql() + * For the tests where we need both fake result and dbhandle info. + * @param string $sql the SQL select query to execute. + * @param array $params array of sql parameters + * @param int $limitfrom return a subset of records, starting at this point (optional). + * @param int $limitnum return a subset comprising this many records (optional, required if $limitfrom is set). + * @return string $handle handle property + */ + public function get_records_sql_p($sql, array $params = null, $limitfrom = 0, $limitnum = 0) { + return parent::get_records_sql($sql, $params); + } + /** * Returns fake recordset * @param string $sql @@ -74,3 +86,46 @@ class read_slave_moodle_database_special extends read_slave_moodle_database { return 1; } } + +/** + * Database recordset mock test class + * + * @package core + * @category dml + * @copyright 2018 Catalyst IT + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class read_slave_moodle_recordset_special extends \moodle_recordset { + /** + * Iterator interface + * @return void + */ + public function close() { + } + /** + * Iterator interface + * @return stdClass + */ + public function current() { + return new stdClass(); + } + /** + * Iterator interface + * @return void + */ + public function next() { + } + /** + * Iterator interface + * @return mixed + */ + public function key() { + } + /** + * Iterator interface + * @return bool + */ + public function valid() { + return false; + } +}