L-electron-Rare/moodle
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

MDL-67667 core: Deprecate and removal task blocking

Browse Source 
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.
This commit is contained in:
Andrew Nicols
2024-04-12 12:34:20 +08:00
parent a1b0fe9b9c
commit 6f1df841f7
19 changed files with 49 additions and 68 deletions
Download Patch File Download Diff File Expand all files Collapse all files
admin/cli/scheduled_task.php
+1 -5
View File
@@ -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);
}
auth/ldap/classes/task/asynchronous_sync_task.php
-2
View File
@@ -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');
}
backup/backup.php
-1
View File
@@ -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);
backup/restore.php
-1
View File
@@ -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);
backup/tests/async_backup_test.php
-3
View File
@@ -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);
backup/tests/async_restore_test.php
-3
View File
@@ -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);
backup/util/helper/backup_cron_helper.class.php
-1
View File
@@ -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
backup/util/helper/copy_helper.class.php
-1
View File
@@ -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);
backup/util/helper/tests/copy_helper_test.php
-1
View File
@@ -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);
}
cache/stores/file/tests/asyncpurge_test.php
Vendored
-1
View File
@@ -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);
course/tests/backup/restore_test.php
-1
View File
@@ -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);
lib/classes/task/manager.php
+2 -26
View File
@@ -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;
lib/classes/task/task_base.php
+16 -8
View File
@@ -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;
}
/**
lib/db/install.xml
+1 -3
View File
@@ -1,5 +1,5 @@
<?xml version="1.0" encoding="UTF-8" ?>
<XMLDB PATH="lib/db" VERSION="20240326" COMMENT="XMLDB file for core Moodle tables"
<XMLDB PATH="lib/db" VERSION="20240408" COMMENT="XMLDB file for core Moodle tables"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:noNamespaceSchemaLocation="../../lib/xmldb/xmldb.xsd"
>
@@ -3475,7 +3475,6 @@
<FIELD NAME="classname" TYPE="char" LENGTH="255" NOTNULL="true" SEQUENCE="false" COMMENT="The class extending scheduled_task to be called when running this task."/>
<FIELD NAME="lastruntime" TYPE="int" LENGTH="10" NOTNULL="false" SEQUENCE="false"/>
<FIELD NAME="nextruntime" TYPE="int" LENGTH="10" NOTNULL="false" SEQUENCE="false"/>
<FIELD NAME="blocking" TYPE="int" LENGTH="2" NOTNULL="true" DEFAULT="0" SEQUENCE="false" COMMENT="Block the entire cron when this task is running."/>
<FIELD NAME="minute" TYPE="char" LENGTH="200" NOTNULL="true" SEQUENCE="false"/>
<FIELD NAME="hour" TYPE="char" LENGTH="70" NOTNULL="true" SEQUENCE="false"/>
<FIELD NAME="day" TYPE="char" LENGTH="90" NOTNULL="true" SEQUENCE="false"/>
@@ -3504,7 +3503,6 @@
<FIELD NAME="faildelay" TYPE="int" LENGTH="10" NOTNULL="false" SEQUENCE="false"/>
<FIELD NAME="customdata" TYPE="text" NOTNULL="false" SEQUENCE="false" COMMENT="Custom data to be passed to the adhoc task. Must be serialisable using json_encode()"/>
<FIELD NAME="userid" TYPE="int" LENGTH="10" NOTNULL="false" SEQUENCE="false"/>
<FIELD NAME="blocking" TYPE="int" LENGTH="2" NOTNULL="true" DEFAULT="0" SEQUENCE="false"/>
<FIELD NAME="timecreated" TYPE="int" LENGTH="10" NOTNULL="true" DEFAULT="0" SEQUENCE="false" COMMENT="Timestamp of adhoc task creation"/>
<FIELD NAME="timestarted" TYPE="int" LENGTH="10" NOTNULL="false" SEQUENCE="false" COMMENT="Time when the task was started"/>
<FIELD NAME="hostname" TYPE="char" LENGTH="255" NOTNULL="false" SEQUENCE="false" COMMENT="Hostname where the task is running"/>
lib/db/upgrade.php
+23
View File
@@ -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;
}
lib/phpunit/classes/advanced_testcase.php
+1 -5
View File
@@ -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);
lib/tests/behat/behat_general.php
+1 -5
View File
@@ -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.
lib/upgrade.txt
+3
View File
@@ -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
version.php
+1 -1
View File
@@ -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