From 6f1df841f7bf35d76ae00ba3ab6e3123e7f38b8e Mon Sep 17 00:00:00 2001 From: Andrew Nicols Date: Mon, 8 Apr 2024 11:47:19 +0800 Subject: [PATCH] MDL-67667 core: Deprecate and removal task blocking There are inherent issues with task blocking which mean that it has never worked properly. It is also very buggy and can lead to massive performance issues with task processing. It is almost impossible to deprecate this in a staged fashion because various APIs use the methods and it is not possible to determine which are API uses and which are other valid uses. In reality there has been little-to-no uptake on the use of this feature and it should just be removed. --- admin/cli/scheduled_task.php | 6 +--- .../classes/task/asynchronous_sync_task.php | 2 -- backup/backup.php | 1 - backup/restore.php | 1 - backup/tests/async_backup_test.php | 3 -- backup/tests/async_restore_test.php | 3 -- .../util/helper/backup_cron_helper.class.php | 1 - backup/util/helper/copy_helper.class.php | 1 - backup/util/helper/tests/copy_helper_test.php | 1 - cache/stores/file/tests/asyncpurge_test.php | 1 - course/tests/backup/restore_test.php | 1 - lib/classes/task/manager.php | 28 ++----------------- lib/classes/task/task_base.php | 24 ++++++++++------ lib/db/install.xml | 4 +-- lib/db/upgrade.php | 23 +++++++++++++++ lib/phpunit/classes/advanced_testcase.php | 6 +--- lib/tests/behat/behat_general.php | 6 +--- lib/upgrade.txt | 3 ++ version.php | 2 +- 19 files changed, 49 insertions(+), 68 deletions(-) diff --git a/admin/cli/scheduled_task.php b/admin/cli/scheduled_task.php index 6c8144b45fb..d75b4f8e0fb 100644 --- a/admin/cli/scheduled_task.php +++ b/admin/cli/scheduled_task.php @@ -189,11 +189,7 @@ if ($disable = $options['disable']) { } $task->set_lock($lock); - if (!$task->is_blocking()) { - $cronlock->release(); - } else { - $task->set_cron_lock($cronlock); - } + $cronlock->release(); \core\cron::run_inner_scheduled_task($task); } diff --git a/auth/ldap/classes/task/asynchronous_sync_task.php b/auth/ldap/classes/task/asynchronous_sync_task.php index b36fb78ca6d..e4d03d0f338 100644 --- a/auth/ldap/classes/task/asynchronous_sync_task.php +++ b/auth/ldap/classes/task/asynchronous_sync_task.php @@ -34,7 +34,6 @@ use core\task\adhoc_task; * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ class asynchronous_sync_task extends adhoc_task { - /** @var string Message prefix for mtrace */ protected const MTRACE_MSG = 'Synced ldap users'; @@ -42,7 +41,6 @@ class asynchronous_sync_task extends adhoc_task { * Constructor */ public function __construct() { - $this->set_blocking(false); $this->set_component('auth_ldap'); } diff --git a/backup/backup.php b/backup/backup.php index 4f2519572ad..b3f597fd9b9 100644 --- a/backup/backup.php +++ b/backup/backup.php @@ -203,7 +203,6 @@ if (!async_helper::is_async_pending($id, 'course', 'backup')) { // Create adhoc task for backup. $asynctask = new \core\task\asynchronous_backup_task(); - $asynctask->set_blocking(false); $asynctask->set_custom_data(array('backupid' => $backupid)); $asynctask->set_userid($USER->id); \core\task\manager::queue_adhoc_task($asynctask); diff --git a/backup/restore.php b/backup/restore.php index e6edbb4efbc..22f05c07d78 100644 --- a/backup/restore.php +++ b/backup/restore.php @@ -173,7 +173,6 @@ if ($restore->get_stage() != restore_ui::STAGE_PROCESS) { // Create adhoc task for restore. $restoreid = $restore->get_restoreid(); $asynctask = new \core\task\asynchronous_restore_task(); - $asynctask->set_blocking(false); $asynctask->set_userid($USER->id); $asynctask->set_custom_data(array('backupid' => $restoreid)); \core\task\manager::queue_adhoc_task($asynctask); diff --git a/backup/tests/async_backup_test.php b/backup/tests/async_backup_test.php index ebb5711ac69..03b412bb43d 100644 --- a/backup/tests/async_backup_test.php +++ b/backup/tests/async_backup_test.php @@ -105,7 +105,6 @@ class async_backup_test extends \advanced_testcase { // Create the adhoc task. $asynctask = new \core\task\asynchronous_backup_task(); - $asynctask->set_blocking(false); $asynctask->set_custom_data(['backupid' => $backupid]); $asynctask->set_userid($USER->id); \core\task\manager::queue_adhoc_task($asynctask); @@ -151,7 +150,6 @@ class async_backup_test extends \advanced_testcase { // Create the adhoc task. $asynctask = new \core\task\asynchronous_backup_task(); - $asynctask->set_blocking(false); $asynctask->set_custom_data(['backupid' => $backupid]); \core\task\manager::queue_adhoc_task($asynctask); @@ -242,7 +240,6 @@ class async_backup_test extends \advanced_testcase { // Now queue an adhoc task and check it handles and completes gracefully. $asynctask = new \core\task\asynchronous_backup_task(); - $asynctask->set_blocking(false); $asynctask->set_custom_data(array('backupid' => $backupid)); \core\task\manager::queue_adhoc_task($asynctask); diff --git a/backup/tests/async_restore_test.php b/backup/tests/async_restore_test.php index 01faf9a1731..f528f9b862d 100644 --- a/backup/tests/async_restore_test.php +++ b/backup/tests/async_restore_test.php @@ -116,7 +116,6 @@ class async_restore_test extends \advanced_testcase { // Create the adhoc task. $asynctask = new \core\task\asynchronous_restore_task(); - $asynctask->set_blocking(false); $asynctask->set_custom_data(array('backupid' => $restoreid)); $asynctask->set_userid($USER->id); \core\task\manager::queue_adhoc_task($asynctask); @@ -223,7 +222,6 @@ class async_restore_test extends \advanced_testcase { // Create the adhoc task. $asynctask = new \core\task\asynchronous_restore_task(); - $asynctask->set_blocking(false); $asynctask->set_custom_data(['backupid' => $restoreid]); \core\task\manager::queue_adhoc_task($asynctask); @@ -245,7 +243,6 @@ class async_restore_test extends \advanced_testcase { // Create the adhoc task. $asynctask = new \core\task\asynchronous_restore_task(); - $asynctask->set_blocking(false); $asynctask->set_custom_data(['backupid' => $restoreid]); \core\task\manager::queue_adhoc_task($asynctask); diff --git a/backup/util/helper/backup_cron_helper.class.php b/backup/util/helper/backup_cron_helper.class.php index ecd1617fa7e..3fdf7ac5770 100644 --- a/backup/util/helper/backup_cron_helper.class.php +++ b/backup/util/helper/backup_cron_helper.class.php @@ -378,7 +378,6 @@ abstract class backup_cron_automated_helper { global $DB; $asynctask = new \core\task\course_backup_task(); - $asynctask->set_blocking(false); $asynctask->set_custom_data(array( 'courseid' => $backupcourse->courseid, 'adminid' => $admin->id diff --git a/backup/util/helper/copy_helper.class.php b/backup/util/helper/copy_helper.class.php index 0bbd042424f..4cbd7d0d70a 100644 --- a/backup/util/helper/copy_helper.class.php +++ b/backup/util/helper/copy_helper.class.php @@ -96,7 +96,6 @@ final class copy_helper { // Create the ad-hoc task to perform the course copy. $asynctask = new \core\task\asynchronous_copy_task(); - $asynctask->set_blocking(false); $asynctask->set_custom_data($copyids); \core\task\manager::queue_adhoc_task($asynctask); diff --git a/backup/util/helper/tests/copy_helper_test.php b/backup/util/helper/tests/copy_helper_test.php index 1080fe56e66..7694343658b 100644 --- a/backup/util/helper/tests/copy_helper_test.php +++ b/backup/util/helper/tests/copy_helper_test.php @@ -333,7 +333,6 @@ class copy_helper_test extends \advanced_testcase { $this->assertInstanceOf('\\core\\task\\asynchronous_copy_task', $task); $this->assertEquals($result, (array)$task->get_custom_data()); - $this->assertFalse($task->is_blocking()); \core\task\manager::adhoc_task_complete($task); } diff --git a/cache/stores/file/tests/asyncpurge_test.php b/cache/stores/file/tests/asyncpurge_test.php index 6611aecf11e..79bb5dac3b5 100644 --- a/cache/stores/file/tests/asyncpurge_test.php +++ b/cache/stores/file/tests/asyncpurge_test.php @@ -86,7 +86,6 @@ class asyncpurge_test extends \advanced_testcase { // Create / execute adhoc task to delete cache revision directory. $asynctask = new cachestore_file\task\asyncpurge(); - $asynctask->set_blocking(false); $asynctask->set_custom_data(['path' => $cacherevdir]); $asynctask->set_userid($USER->id); \core\task\manager::queue_adhoc_task($asynctask); diff --git a/course/tests/backup/restore_test.php b/course/tests/backup/restore_test.php index 85bf9004d24..e364957740c 100644 --- a/course/tests/backup/restore_test.php +++ b/course/tests/backup/restore_test.php @@ -159,7 +159,6 @@ class restore_test extends \advanced_testcase { // Create the adhoc task. $asynctask = new \core\task\asynchronous_restore_task(); - $asynctask->set_blocking(false); $asynctask->set_custom_data(array('backupid' => $restoreid)); \core\task\manager::queue_adhoc_task($asynctask); diff --git a/lib/classes/task/manager.php b/lib/classes/task/manager.php index 8385172d9e0..5aba3f187c9 100644 --- a/lib/classes/task/manager.php +++ b/lib/classes/task/manager.php @@ -304,7 +304,6 @@ class manager { $record = new \stdClass(); $record->classname = self::get_canonical_class_name($task); $record->component = $task->get_component(); - $record->blocking = $task->is_blocking(); $record->customised = $task->is_customised(); $record->lastruntime = $task->get_last_run_time(); $record->nextruntime = $task->get_next_run_time(); @@ -333,7 +332,6 @@ class manager { $record->classname = self::get_canonical_class_name($task); $record->id = $task->get_id(); $record->component = $task->get_component(); - $record->blocking = $task->is_blocking(); $record->nextruntime = $task->get_next_run_time(); $record->faildelay = $task->get_fail_delay(); $record->customdata = $task->get_custom_data_as_string(); @@ -368,7 +366,6 @@ class manager { if (isset($record->component)) { $task->set_component($record->component); } - $task->set_blocking(!empty($record->blocking)); if (isset($record->faildelay)) { $task->set_fail_delay($record->faildelay); } @@ -430,7 +427,6 @@ class manager { if (isset($record->component)) { $task->set_component($record->component); } - $task->set_blocking(!empty($record->blocking)); if (isset($record->minute)) { $task->set_minute($record->minute, $expandr); } @@ -1040,11 +1036,7 @@ class manager { } $task->set_lock($lock); - if (!$task->is_blocking()) { - $cronlock->release(); - } else { - $task->set_cron_lock($cronlock); - } + $cronlock->release(); } /** @@ -1107,11 +1099,7 @@ class manager { throw new \moodle_exception('locktimeout'); } - if (!$task->is_blocking()) { - $cronlock->release(); - } else { - $task->set_cron_lock($cronlock); - } + $cronlock->release(); return $task; } } @@ -1198,9 +1186,6 @@ class manager { $DB->update_record('task_adhoc', $record); $task->release_concurrency_lock(); - if ($task->is_blocking()) { - $task->get_cron_lock()->release(); - } $task->get_lock()->release(); self::$runningtask = null; @@ -1259,9 +1244,6 @@ class manager { // Release the locks. $task->release_concurrency_lock(); - if ($task->is_blocking()) { - $task->get_cron_lock()->release(); - } $task->get_lock()->release(); self::$runningtask = null; @@ -1311,9 +1293,6 @@ class manager { $record->pid = null; $DB->update_record('task_scheduled', $record); - if ($task->is_blocking()) { - $task->get_cron_lock()->release(); - } $task->get_lock()->release(); self::$runningtask = null; @@ -1394,9 +1373,6 @@ class manager { } // Reschedule and then release the locks. - if ($task->is_blocking()) { - $task->get_cron_lock()->release(); - } $task->get_lock()->release(); self::$runningtask = null; diff --git a/lib/classes/task/task_base.php b/lib/classes/task/task_base.php index 1f3c0eed42f..8253389ef88 100644 --- a/lib/classes/task/task_base.php +++ b/lib/classes/task/task_base.php @@ -45,9 +45,6 @@ abstract class task_base { /** @var string $component - The component this task belongs to. */ private $component = ''; - /** @var bool $blocking - Does this task block the entire cron process. */ - private $blocking = false; - /** @var int $faildelay - Exponentially increasing fail delay */ private $faildelay = 0; @@ -120,10 +117,21 @@ abstract class task_base { /** * Setter for $blocking. - * @param bool $blocking + * + * Please note that task blocking is no longer supported. + * If you are using it in older versions of Moodle you are strongly advised to rewrite your code + * as has a detrimental impact upon performance. + * + * @deprecated since Moodle 4.4 See MDL-67667 + * @todo Remove in MDL-81509 */ + #[\core\attribute\deprecated( + replacement: null, + since: '4.4', + reason: 'Blocking tasks are no longer supported', + )] public function set_blocking($blocking) { - $this->blocking = $blocking; + \core\deprecation::emit_deprecation_if_present([$this, __FUNCTION__]); } /** @@ -131,16 +139,16 @@ abstract class task_base { * * @return bool * @deprecated since Moodle 4.4 See MDL-67667 + * @todo Remove in MDL-81509 */ #[\core\attribute\deprecated( replacement: null, since: '4.4', reason: 'Blocking tasks are no longer supported', - emit: false, )] public function is_blocking() { - \core\deprecation::emit_deprecation_if_present(([$this, __FUNCTION__]); - return $this->blocking; + \core\deprecation::emit_deprecation_if_present([$this, __FUNCTION__]); + return false; } /** diff --git a/lib/db/install.xml b/lib/db/install.xml index 41345e8e814..a2f52c5a996 100644 --- a/lib/db/install.xml +++ b/lib/db/install.xml @@ -1,5 +1,5 @@ - @@ -3475,7 +3475,6 @@ - @@ -3504,7 +3503,6 @@ - diff --git a/lib/db/upgrade.php b/lib/db/upgrade.php index c04f124cbb1..297e906c794 100644 --- a/lib/db/upgrade.php +++ b/lib/db/upgrade.php @@ -1144,5 +1144,28 @@ function xmldb_main_upgrade($oldversion) { upgrade_main_savepoint(true, 2024032600.01); } + if ($oldversion < 2024041200.00) { + // Define field blocking to be dropped from task_adhoc. + $table = new xmldb_table('task_adhoc'); + $field = new xmldb_field('blocking'); + + // Conditionally launch drop field customdata. + if ($dbman->field_exists($table, $field)) { + $dbman->drop_field($table, $field); + } + + // Define field blocking to be dropped from task_scheduled. + $table = new xmldb_table('task_scheduled'); + $field = new xmldb_field('blocking'); + + // Conditionally launch drop field customdata. + if ($dbman->field_exists($table, $field)) { + $dbman->drop_field($table, $field); + } + + // Main savepoint reached. + upgrade_main_savepoint(true, 2024041200.00); + } + return true; } diff --git a/lib/phpunit/classes/advanced_testcase.php b/lib/phpunit/classes/advanced_testcase.php index dca7c404c4f..2068ce650fd 100644 --- a/lib/phpunit/classes/advanced_testcase.php +++ b/lib/phpunit/classes/advanced_testcase.php @@ -705,11 +705,7 @@ abstract class advanced_testcase extends base_testcase { } $task->set_lock($lock); - if (!$task->is_blocking()) { - $cronlock->release(); - } else { - $task->set_cron_lock($cronlock); - } + $cronlock->release(); \core\cron::prepare_core_renderer(); \core\cron::setup_user($user); diff --git a/lib/tests/behat/behat_general.php b/lib/tests/behat/behat_general.php index a2468e1ca12..b38bd60956d 100644 --- a/lib/tests/behat/behat_general.php +++ b/lib/tests/behat/behat_general.php @@ -1176,11 +1176,7 @@ EOF; throw new DriverException('Unable to obtain task lock for scheduled task'); } $task->set_lock($lock); - if (!$task->is_blocking()) { - $cronlock->release(); - } else { - $task->set_cron_lock($cronlock); - } + $cronlock->release(); try { // Prepare the renderer. diff --git a/lib/upgrade.txt b/lib/upgrade.txt index 09a2ec88eb5..9323bed124f 100644 --- a/lib/upgrade.txt +++ b/lib/upgrade.txt @@ -35,6 +35,9 @@ information provided here is intended especially for developers. By default, tasks will be retried until they succeed, other tasks can override this method to change this behaviour. - set_attempts_available(): Used to set the number of attempts available for the task - get_attempts_available(): Used to get the number of attempts available for the task. +* Support for blocking tasks has been removed. See MDL-67667 for further information. + Please note that this feature never worked correctly and can cause serious performance issues. + It is also not possible to deprecate this feature in a notifiable way. * There is a new DML method $DB->get_fieldset. For some reason, this did not exist even though get_fieldset_select etc. did. * The following callbacks have been migrated to hooks: - before_standard_html_head() -> core\hook\output\before_standard_head_html_generation diff --git a/version.php b/version.php index 57907b6b549..743f03c7b0c 100644 --- a/version.php +++ b/version.php @@ -29,7 +29,7 @@ defined('MOODLE_INTERNAL') || die(); -$version = 2024040900.00; // YYYYMMDD = weekly release date of this DEV branch. +$version = 2024041200.00; // YYYYMMDD = weekly release date of this DEV branch. // RR = release increments - 00 in DEV branches. // .XX = incremental changes. $release = '4.4dev+ (Build: 20240409)'; // Human-friendly version name