diff --git a/lib/classes/lock/lock_utils.php b/lib/classes/lock/lock_utils.php new file mode 100644 index 00000000000..1263591dc0f --- /dev/null +++ b/lib/classes/lock/lock_utils.php @@ -0,0 +1,76 @@ +. + +namespace core\lock; + +use core\progress\base; + +/** + * Lock utilities. + * + * @package core + * @copyright 2023 onwards Catalyst IT EU {@link https://catalyst-eu.net} + * @author Mark Johnson + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class lock_utils { + /** + * Start a progress bar and attempt to get a lock, updating the bar until a lock is achieved. + * + * This will make multiple attempts at getting the lock using a short timeout set by $progressupdatetime. After + * each failed attempt, it will update the progress bar and try again, until $timeout is reached. + * + * @param lock_factory $lockfactory The factory to use to get the lock + * @param string $resource The resource key we will try to get a lock on + * @param base $progress The progress bar + * @param int $timeout The maximum time in seconds to wait for a lock + * @param string $message Optional message to display on the progress bar + * @param int $progressupdatetime The number of seconds to wait for each lock attempt before updating the progress bar. + * @param int $maxlifetime The maxlifetime to set on the lock, if supported. + * @return lock|false A lock if successful, or false if the timeout expires. + * @throws \coding_exception + */ + public static function wait_for_lock_with_progress( + lock_factory $lockfactory, + string $resource, + \core\progress\base $progress, + int $timeout, + string $message = '', + int $progressupdatetime = 10, + int $maxlifetime = DAYSECS, + ) { + if ($progressupdatetime < 1) { + throw new \invalid_parameter_exception('Progress bar cannot update more than once per second. ' . + '$progressupdate time must be at least 1.'); + } + if ($progressupdatetime > $timeout) { + throw new \invalid_parameter_exception('$timeout must be greater than $progressupdatetime.'); + } + $lockattempts = 0; + $maxattempts = $timeout / $progressupdatetime; + $lock = false; + $progress->start_progress($message); + while (!$lock && $lockattempts < $maxattempts) { + $lock = $lockfactory->get_lock($resource, $progressupdatetime, $maxlifetime); + if (!$lock) { + $progress->progress(); + $lockattempts++; + } + } + $progress->end_progress(); + return $lock; + } +} diff --git a/mod/quiz/lib.php b/mod/quiz/lib.php index 08841dd0730..ec05da4251b 100644 --- a/mod/quiz/lib.php +++ b/mod/quiz/lib.php @@ -2518,5 +2518,5 @@ function mod_quiz_calculate_question_stats(context $context): ?all_calculated_fo require_once($CFG->dirroot . '/mod/quiz/report/statistics/report.php'); $cm = get_coursemodule_from_id('quiz', $context->instanceid); $report = new quiz_statistics_report(); - return $report->calculate_questions_stats_for_question_bank($cm->instance, false); + return $report->calculate_questions_stats_for_question_bank($cm->instance, false, false); } diff --git a/mod/quiz/report/statistics/lang/en/quiz_statistics.php b/mod/quiz/report/statistics/lang/en/quiz_statistics.php index e9da802cd05..d4ed1e6c329 100644 --- a/mod/quiz/report/statistics/lang/en/quiz_statistics.php +++ b/mod/quiz/report/statistics/lang/en/quiz_statistics.php @@ -62,6 +62,10 @@ $string['firstattempts'] = 'first attempts'; $string['firstattemptsavg'] = 'Average grade of first attempts'; $string['firstattemptscount'] = 'Number of complete graded first attempts'; $string['frequency'] = 'Frequency'; +$string['getstatslockprogress'] = 'Waiting for task in progress. Please wait or try again later.'; +$string['getstatslocktimeout'] = 'Statistics calculation lock timeout'; +$string['getstatslocktimeoutdesc'] = 'How many seconds to wait for a lock when attempting to perform a statistics calculation ' . + 'for a quiz. This setting primarily exists for testing, do not modify it unless you know what you are doing.'; $string['highestattempts'] = 'highest graded attempt'; $string['highestattemptsavg'] = 'Average grade of highest graded attempts'; $string['intended_weight'] = 'Intended weight'; @@ -83,6 +87,7 @@ Our equation for effective question weight cannot be calculated in this case. Th If you edit a quiz and give these question(s) with negative covariance a max grade of zero then the effective question weight of these questions will be zero and the real effective question weight of other questions will be as calculated now.'; $string['nogradedattempts'] = 'No attempts have been made at this quiz, or all attempts have questions that need manual grading.'; $string['nostudentsingroup'] = 'There are no students in this group yet'; +$string['nostats'] = 'Could not complete the statistics calculation. There may be a long-running calculation in progress. Please try again later.'; $string['optiongrade'] = 'Partial credit'; $string['partofquestion'] = 'Part of question'; $string['pluginname'] = 'Statistics'; diff --git a/mod/quiz/report/statistics/report.php b/mod/quiz/report/statistics/report.php index 05293762288..116b9b816df 100644 --- a/mod/quiz/report/statistics/report.php +++ b/mod/quiz/report/statistics/report.php @@ -157,6 +157,10 @@ class quiz_statistics_report extends report_base { $progress = $this->get_progress_trace_instance(); list($quizstats, $questionstats) = $this->get_all_stats_and_analysis($quiz, $whichattempts, $whichtries, $groupstudentsjoins, $questions, $progress); + if (is_null($quizstats)) { + echo $OUTPUT->notification(get_string('nostats', 'quiz_statistics'), 'error'); + return true; + } } else { // Or create empty stats containers. $quizstats = new \quiz_statistics\calculated($whichattempts); @@ -624,13 +628,15 @@ class quiz_statistics_report extends report_base { * @param \core\progress\base|null $progress * @param bool $calculateifrequired if true (the default) the stats will be calculated if not already stored. * If false, [null, null] will be returned if the stats are not already available. + * @param bool $performanalysis if true (the default) and there are calculated stats, analysis will be performed + * for each question. * @return array with 2 elements: - $quizstats The statistics for overall attempt scores. * - $questionstats \core_question\statistics\questions\all_calculated_for_qubaid_condition * Both may be null, if $calculateifrequired is false. */ public function get_all_stats_and_analysis( $quiz, $whichattempts, $whichtries, \core\dml\sql_join $groupstudentsjoins, - $questions, $progress = null, bool $calculateifrequired = true) { + $questions, $progress = null, bool $calculateifrequired = true, bool $performanalysis = true) { if ($progress === null) { $progress = new \core\progress\none(); @@ -642,38 +648,82 @@ class quiz_statistics_report extends report_base { $quizcalc = new \quiz_statistics\calculator($progress); - $progress->start_progress('', 3); - if ($quizcalc->get_last_calculated_time($qubaids) === false) { + $progress->start_progress('', 4); + + // Get a lock on this set of qubaids before performing calculations. This prevents the same calculation running + // concurrently and causing database deadlocks. We use a long timeout here as a big quiz with lots of attempts may + // take a long time to process. + $lockfactory = \core\lock\lock_config::get_lock_factory('quiz_statistics_get_stats'); + $lock = $lockfactory->get_lock($qubaids->get_hash_code(), 0); + if (!$lock) { if (!$calculateifrequired) { - $progress->progress(3); + // We're not going to do the calculation in this request anyway, so just give up here. + $progress->progress(4); $progress->end_progress(); return [null, null]; } - - // Recalculate now. - $questionstats = $qcalc->calculate($qubaids); - $progress->progress(1); - - $quizstats = $quizcalc->calculate($quiz->id, $whichattempts, $groupstudentsjoins, count($questions), - $qcalc->get_sum_of_mark_variance()); - $progress->progress(2); - } else { - $quizstats = $quizcalc->get_cached($qubaids); - $progress->progress(1); - $questionstats = $qcalc->get_cached($qubaids); - $progress->progress(2); + $locktimeout = get_config('quiz_statistics', 'getstatslocktimeout'); + $lock = \core\lock\lock_utils::wait_for_lock_with_progress( + $lockfactory, + $qubaids->get_hash_code(), + $progress, + $locktimeout, + get_string('getstatslockprogress', 'quiz_statistics'), + ); + if (!$lock) { + // Lock attempt timed out. + $progress->progress(4); + $progress->end_progress(); + debugging('Could not get lock on ' . + $qubaids->get_hash_code() . ' (Quiz ID ' . $quiz->id . ') after ' . + $locktimeout . ' seconds'); + return [null, null]; + } } - if ($quizstats->s()) { - $subquestions = $questionstats->get_sub_questions(); - $this->analyse_responses_for_all_questions_and_subquestions($questions, - $subquestions, - $qubaids, - $whichtries, - $progress); + try { + if ($quizcalc->get_last_calculated_time($qubaids) === false) { + if (!$calculateifrequired) { + $progress->progress(4); + $progress->end_progress(); + $lock->release(); + return [null, null]; + } + + // Recalculate now. + $questionstats = $qcalc->calculate($qubaids); + $progress->progress(2); + + $quizstats = $quizcalc->calculate( + $quiz->id, + $whichattempts, + $groupstudentsjoins, + count($questions), + $qcalc->get_sum_of_mark_variance() + ); + $progress->progress(3); + } else { + $quizstats = $quizcalc->get_cached($qubaids); + $progress->progress(2); + $questionstats = $qcalc->get_cached($qubaids); + $progress->progress(3); + } + + if ($quizstats->s() && $performanalysis) { + $subquestions = $questionstats->get_sub_questions(); + $this->analyse_responses_for_all_questions_and_subquestions( + $questions, + $subquestions, + $qubaids, + $whichtries, + $progress + ); + } + $progress->progress(4); + $progress->end_progress(); + } finally { + $lock->release(); } - $progress->progress(3); - $progress->end_progress(); return [$quizstats, $questionstats]; } @@ -932,11 +982,14 @@ class quiz_statistics_report extends report_base { * @param int $quizid question usage * @param bool $calculateifrequired if true (the default) the stats will be calculated if not already stored. * If false, null will be returned if the stats are not already available. + * @param bool $performanalysis if true (the default) and there are calculated stats, analysis will be performed + * for each question. * @return ?all_calculated_for_qubaid_condition question stats */ public function calculate_questions_stats_for_question_bank( int $quizid, - bool $calculateifrequired = true + bool $calculateifrequired = true, + bool $performanalysis = true, ): ?all_calculated_for_qubaid_condition { global $DB; $quiz = $DB->get_record('quiz', ['id' => $quizid], '*', MUST_EXIST); @@ -944,7 +997,7 @@ class quiz_statistics_report extends report_base { [, $questionstats] = $this->get_all_stats_and_analysis($quiz, $quiz->grademethod, question_attempt::ALL_TRIES, new \core\dml\sql_join(), - $questions, null, $calculateifrequired); + $questions, null, $calculateifrequired, $performanalysis); return $questionstats; } diff --git a/mod/quiz/report/statistics/settings.php b/mod/quiz/report/statistics/settings.php new file mode 100644 index 00000000000..7aafb20b625 --- /dev/null +++ b/mod/quiz/report/statistics/settings.php @@ -0,0 +1,35 @@ +. + +/** + * Settings for the Statisics report + * + * @package quiz_statistics + * @copyright 2023 onwards Catalyst IT EU {@link https://catalyst-eu.net} + * @author Mark Johnson + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +defined('MOODLE_INTERNAL') || die; + +if ($ADMIN->fulltree) { + $settings->add(new admin_setting_configtext( + 'quiz_statistics/getstatslocktimeout', + get_string('getstatslocktimeout', 'quiz_statistics'), + get_string('getstatslocktimeoutdesc', 'quiz_statistics'), + MINSECS * 15 + )); +} diff --git a/mod/quiz/report/statistics/tests/test_quiz_statistics_report.php b/mod/quiz/report/statistics/tests/test_quiz_statistics_report.php new file mode 100644 index 00000000000..42b1f5e05c7 --- /dev/null +++ b/mod/quiz/report/statistics/tests/test_quiz_statistics_report.php @@ -0,0 +1,212 @@ +. + +namespace quiz_statistics; + +defined('MOODLE_INTERNAL') || die(); + +global $CFG; +require_once($CFG->dirroot . '/mod/quiz/report/statistics/report.php'); +require_once($CFG->dirroot . '/mod/quiz/report/statistics/statisticslib.php'); +require_once($CFG->dirroot . '/mod/quiz/report/reportlib.php'); +require_once($CFG->dirroot . '/mod/quiz/tests/quiz_question_helper_test_trait.php'); + +/** + * Tests for statistics report + * + * @package quiz_statistics + * @copyright 2023 onwards Catalyst IT EU {@link https://catalyst-eu.net} + * @author Mark Johnson + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @covers \quiz_statistics_report + */ +class test_quiz_statistics_report extends \advanced_testcase { + + use \quiz_question_helper_test_trait; + + /** + * Secondary database connection for creating locks. + * + * @var \moodle_database|null + */ + protected static ?\moodle_database $lockdb; + + /** + * Lock factory using the secondary database connection. + * + * @var \moodle_database|null + */ + protected static ?\core\lock\lock_factory $lockfactory; + + /** + * Create a lock factory with a second database session. + * + * This allows us to create a lock in our test code that will block a lock request + * on the same key in code under test. + * + * @return void + */ + public static function setUpBeforeClass(): void { + global $CFG; + self::$lockdb = \moodle_database::get_driver_instance($CFG->dbtype, $CFG->dblibrary); + self::$lockdb->connect($CFG->dbhost, $CFG->dbuser, $CFG->dbpass, $CFG->dbname, $CFG->prefix, $CFG->dboptions); + $lockfactory = \core\lock\lock_config::get_lock_factory('quiz_statistics_get_stats'); + $reflectiondb = new \ReflectionProperty($lockfactory, 'db'); + $reflectiondb->setAccessible(true); + $reflectiondb->setValue($lockfactory, self::$lockdb); + self::$lockfactory = $lockfactory; + } + + /** + * Dispose of the extra DB connection and lock factory. + * + * @return void + */ + public static function tearDownAfterClass(): void { + self::$lockdb->dispose(); + self::$lockdb = null; + self::$lockfactory = null; + } + + /** + * Return a generated quiz + * + * @return \stdClass + */ + protected function create_and_attempt_quiz(): \stdClass { + $course = $this->getDataGenerator()->create_course(); + $user = $this->getDataGenerator()->create_user(); + $quiz = $this->create_test_quiz($course); + $quizcontext = \context_module::instance($quiz->cmid); + $questiongenerator = $this->getDataGenerator()->get_plugin_generator('core_question'); + $this->add_two_regular_questions($questiongenerator, $quiz, ['contextid' => $quizcontext->id]); + $this->attempt_quiz($quiz, $user); + + return $quiz; + } + + /** + * Test locking the calculation process. + * + * When there is a lock on the hash code, test_get_all_stats_and_analysis() should wait until the lock timeout, then throw an + * exception. + * + * When there is no lock (or the lock has been released), it should return a result. + * + * @return void + */ + public function test_get_all_stats_and_analysis_locking(): void { + $this->resetAfterTest(true); + $quiz = $this->create_and_attempt_quiz(); + $whichattempts = QUIZ_GRADEAVERAGE; // All attempts. + $whichtries = \question_attempt::ALL_TRIES; + $groupstudentsjoins = new \core\dml\sql_join(); + $qubaids = quiz_statistics_qubaids_condition($quiz->id, $groupstudentsjoins, $whichattempts); + + $report = new \quiz_statistics_report(); + $questions = $report->load_and_initialise_questions_for_calculations($quiz); + + $timeoutseconds = 20; + set_config('getstatslocktimeout', $timeoutseconds, 'quiz_statistics'); + $lock = self::$lockfactory->get_lock($qubaids->get_hash_code(), 0); + + $progress = new \core\progress\none(); + + $this->resetDebugging(); + $timebefore = microtime(true); + try { + $result = $report->get_all_stats_and_analysis( + $quiz, + $whichattempts, + $whichtries, + $groupstudentsjoins, + $questions, + $progress + ); + $timeafter = microtime(true); + + // Verify that we waited as long as the timeout. + $this->assertEqualsWithDelta($timeoutseconds, $timeafter - $timebefore, 1); + $this->assertDebuggingCalled('Could not get lock on ' . + $qubaids->get_hash_code() . ' (Quiz ID ' . $quiz->id . ') after ' . + $timeoutseconds . ' seconds'); + $this->assertEquals([null, null], $result); + } finally { + $lock->release(); + } + + $this->resetDebugging(); + $result = $report->get_all_stats_and_analysis( + $quiz, + $whichattempts, + $whichtries, + $groupstudentsjoins, + $questions + ); + $this->assertDebuggingNotCalled(); + $this->assertNotEquals([null, null], $result); + } + + /** + * Test locking when the current page does not require calculations. + * + * When there is a lock on the hash code, test_get_all_stats_and_analysis() should return a null result immediately, + * with no exception thrown. + * + * @return void + */ + public function test_get_all_stats_and_analysis_locking_no_calculation(): void { + $this->resetAfterTest(true); + $quiz = $this->create_and_attempt_quiz(); + + $whichattempts = QUIZ_GRADEAVERAGE; // All attempts. + $whichtries = \question_attempt::ALL_TRIES; + $groupstudentsjoins = new \core\dml\sql_join(); + $qubaids = quiz_statistics_qubaids_condition($quiz->id, $groupstudentsjoins, $whichattempts); + + $report = new \quiz_statistics_report(); + $questions = $report->load_and_initialise_questions_for_calculations($quiz); + + $timeoutseconds = 20; + set_config('getstatslocktimeout', $timeoutseconds, 'quiz_statistics'); + + $lock = self::$lockfactory->get_lock($qubaids->get_hash_code(), 0); + + $this->resetDebugging(); + try { + $progress = new \core\progress\none(); + + $timebefore = microtime(true); + $result = $report->get_all_stats_and_analysis( + $quiz, + $whichattempts, + $whichtries, + $groupstudentsjoins, + $questions, + $progress, + false + ); + $timeafter = microtime(true); + + // Verify that we did not wait for the timeout before returning. + $this->assertLessThan($timeoutseconds, $timeafter - $timebefore); + $this->assertEquals([null, null], $result); + $this->assertDebuggingNotCalled(); + } finally { + $lock->release(); + } + } +} diff --git a/mod/quiz/report/statistics/version.php b/mod/quiz/report/statistics/version.php index 6b369801c9f..9d49cde1a51 100644 --- a/mod/quiz/report/statistics/version.php +++ b/mod/quiz/report/statistics/version.php @@ -24,6 +24,6 @@ defined('MOODLE_INTERNAL') || die(); -$plugin->version = 2023042403; +$plugin->version = 2023042404; $plugin->requires = 2023041800; $plugin->component = 'quiz_statistics'; diff --git a/question/classes/statistics/questions/calculator.php b/question/classes/statistics/questions/calculator.php index 64a673e4919..b4f3736a595 100644 --- a/question/classes/statistics/questions/calculator.php +++ b/question/classes/statistics/questions/calculator.php @@ -257,10 +257,9 @@ class calculator { } } $this->stats->cache($qubaids); - - // All finished. - $this->progress->end_progress(); } + // All finished. + $this->progress->end_progress(); return $this->stats; }