From d03f36ebeef8a7e3bd72d42935b9c21d6e4c3ab0 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Wed, 30 May 2018 12:20:10 +0100 Subject: [PATCH 01/12] First look at splitting up AsyncPool and ServerScheduler This commit contains quite a few breaking changes with respect to how AsyncTasks are handled. This is necessary to allow separation of the ServerScheduler and the AsyncPool, because in the future the ServerScheduler may be removed and instead there will be isolated per-plugin sync-task schedulers - but we cannot have every plugin with its own worker pool for memory usage reasons if nothing else. The following things have changed: - ServerScheduler: scheduleAsyncTask(), scheduleAsyncTaskToWorker(), getAsyncTaskPoolSize(), increaseAsyncTaskPoolSize() and similar methods have all been removed. Additionally the static \$WORKERS field has been removed. - Server: added API method getAsyncPool(). This grants you direct access to the server's AsyncPool. Calls to getScheduler()->scheduleAsyncTask() and scheduleAsyncTaskToWorker() should be replaced with getAsyncPool()->submitTask() and submitTaskToWorker() respectively. --- src/pocketmine/MemoryManager.php | 12 ++--- src/pocketmine/Player.php | 2 +- src/pocketmine/Server.php | 32 +++++++++--- .../command/defaults/TimingsCommand.php | 2 +- src/pocketmine/level/Level.php | 18 +++---- src/pocketmine/scheduler/AsyncPool.php | 21 ++++---- src/pocketmine/scheduler/ServerScheduler.php | 52 ------------------- src/pocketmine/updater/AutoUpdater.php | 2 +- src/pocketmine/utils/Config.php | 2 +- tests/plugins/PocketMine-TesterPlugin | 2 +- 10 files changed, 54 insertions(+), 91 deletions(-) diff --git a/src/pocketmine/MemoryManager.php b/src/pocketmine/MemoryManager.php index cbf214ce8..ade465a1a 100644 --- a/src/pocketmine/MemoryManager.php +++ b/src/pocketmine/MemoryManager.php @@ -243,9 +243,9 @@ class MemoryManager{ Timings::$garbageCollectorTimer->startTiming(); if($this->garbageCollectionAsync){ - $size = $this->server->getScheduler()->getAsyncTaskPoolSize(); - for($i = 0; $i < $size; ++$i){ - $this->server->getScheduler()->scheduleAsyncTaskToWorker(new GarbageCollectionTask(), $i); + $pool = $this->server->getAsyncPool(); + for($i = 0, $size = $pool->getSize(); $i < $size; ++$i){ + $pool->submitTaskToWorker(new GarbageCollectionTask(), $i); } } @@ -268,9 +268,9 @@ class MemoryManager{ self::dumpMemory($this->server, $outputFolder, $maxNesting, $maxStringSize); if($this->dumpWorkers){ - $scheduler = $this->server->getScheduler(); - for($i = 0, $size = $scheduler->getAsyncTaskPoolSize(); $i < $size; ++$i){ - $scheduler->scheduleAsyncTaskToWorker(new DumpWorkerMemoryTask($outputFolder, $maxNesting, $maxStringSize), $i); + $pool = $this->server->getAsyncPool(); + for($i = 0, $size = $pool->getSize(); $i < $size; ++$i){ + $pool->submitTaskToWorker(new DumpWorkerMemoryTask($outputFolder, $maxNesting, $maxStringSize), $i); } } } diff --git a/src/pocketmine/Player.php b/src/pocketmine/Player.php index 67bf3c78d..04e010b42 100644 --- a/src/pocketmine/Player.php +++ b/src/pocketmine/Player.php @@ -1916,7 +1916,7 @@ class Player extends Human implements CommandSender, ChunkLoader, IPlayer{ } if(!$packet->skipVerification){ - $this->server->getScheduler()->scheduleAsyncTask(new VerifyLoginTask($this, $packet)); + $this->server->getAsyncPool()->submitTask(new VerifyLoginTask($this, $packet)); }else{ $this->onVerifyCompleted($packet, null, true); } diff --git a/src/pocketmine/Server.php b/src/pocketmine/Server.php index f90556ee1..f60675df0 100644 --- a/src/pocketmine/Server.php +++ b/src/pocketmine/Server.php @@ -87,6 +87,7 @@ use pocketmine\plugin\PluginLoadOrder; use pocketmine\plugin\PluginManager; use pocketmine\plugin\ScriptPluginLoader; use pocketmine\resourcepacks\ResourcePackManager; +use pocketmine\scheduler\AsyncPool; use pocketmine\scheduler\FileWriteTask; use pocketmine\scheduler\SendUsageTask; use pocketmine\scheduler\ServerScheduler; @@ -150,6 +151,8 @@ class Server{ /** @var ServerScheduler */ private $scheduler = null; + /** @var AsyncPool */ + private $asyncPool; /** * Counts the ticks since the server start @@ -657,6 +660,10 @@ class Server{ return $this->scheduler; } + public function getAsyncPool() : AsyncPool{ + return $this->asyncPool; + } + /** * @return int */ @@ -824,7 +831,7 @@ class Server{ $nbt = new BigEndianNBTStream(); try{ if($async){ - $this->getScheduler()->scheduleAsyncTask(new FileWriteTask($this->getDataPath() . "players/" . strtolower($name) . ".dat", $nbt->writeCompressed($ev->getSaveData()))); + $this->asyncPool->submitTask(new FileWriteTask($this->getDataPath() . "players/" . strtolower($name) . ".dat", $nbt->writeCompressed($ev->getSaveData()))); }else{ file_put_contents($this->getDataPath() . "players/" . strtolower($name) . ".dat", $nbt->writeCompressed($ev->getSaveData())); } @@ -1518,8 +1525,10 @@ class Server{ $this->logger->info($this->getLanguage()->translateString("pocketmine.server.start", [TextFormat::AQUA . $this->getVersion() . TextFormat::RESET])); + $this->scheduler = new ServerScheduler(); + if(($poolSize = $this->getProperty("settings.async-workers", "auto")) === "auto"){ - $poolSize = ServerScheduler::$WORKERS; + $poolSize = 2; $processors = Utils::getCoreCount() - 2; if($processors > 0){ @@ -1529,7 +1538,7 @@ class Server{ $poolSize = (int) $poolSize; } - ServerScheduler::$WORKERS = $poolSize; + $this->asyncPool = new AsyncPool($this, $poolSize); if($this->getProperty("network.batch-threshold", 256) >= 0){ Network::$BATCH_THRESHOLD = (int) $this->getProperty("network.batch-threshold", 256); @@ -1551,7 +1560,6 @@ class Server{ $this->doTitleTick = ((bool) $this->getProperty("console.title-tick", true)) && Terminal::hasFormattingCodes(); - $this->scheduler = new ServerScheduler(); if($this->getConfigBool("enable-rcon", false)){ try{ @@ -1903,7 +1911,7 @@ class Server{ if(!$forceSync and !$immediate and $this->networkCompressionAsync){ $task = new CompressBatchedTask($pk, $targets); - $this->getScheduler()->scheduleAsyncTask($task); + $this->asyncPool->submitTask($task); }else{ $this->broadcastPacketsCallback($pk, $targets, $immediate); } @@ -2068,8 +2076,12 @@ class Server{ HandlerList::unregisterAll(); if($this->scheduler instanceof ServerScheduler){ - $this->getLogger()->debug("Shutting down task scheduler"); - $this->scheduler->shutdown(); + $this->getLogger()->debug("Stopping all tasks"); + $this->scheduler->cancelAllTasks(); + } + if($this->asyncPool instanceof AsyncPool){ + $this->getLogger()->debug("Shutting down async task worker pool"); + $this->asyncPool->shutdown(); } if($this->properties !== null and $this->properties->hasChanged()){ @@ -2427,7 +2439,7 @@ class Server{ public function sendUsage($type = SendUsageTask::TYPE_STATUS){ if((bool) $this->getProperty("anonymous-statistics.enabled", true)){ - $this->scheduler->scheduleAsyncTask(new SendUsageTask($this, $type, $this->uniquePlayers)); + $this->asyncPool->submitTask(new SendUsageTask($this, $type, $this->uniquePlayers)); } $this->uniquePlayers = []; } @@ -2525,6 +2537,10 @@ class Server{ $this->scheduler->mainThreadHeartbeat($this->tickCounter); Timings::$schedulerTimer->stopTiming(); + Timings::$schedulerAsyncTimer->startTiming(); + $this->asyncPool->collectTasks(); + Timings::$schedulerAsyncTimer->stopTiming(); + $this->checkTickUpdates($this->tickCounter, $tickTime); foreach($this->players as $player){ diff --git a/src/pocketmine/command/defaults/TimingsCommand.php b/src/pocketmine/command/defaults/TimingsCommand.php index ca461268a..3ca5d920f 100644 --- a/src/pocketmine/command/defaults/TimingsCommand.php +++ b/src/pocketmine/command/defaults/TimingsCommand.php @@ -104,7 +104,7 @@ class TimingsCommand extends VanillaCommand{ ]; fclose($fileTimings); - $sender->getServer()->getScheduler()->scheduleAsyncTask(new class([ + $sender->getServer()->getAsyncPool()->submitTask(new class([ ["page" => "http://paste.ubuntu.com", "extraOpts" => [ CURLOPT_HTTPHEADER => ["User-Agent: " . $sender->getServer()->getName() . " " . $sender->getServer()->getPocketMineVersion()], CURLOPT_POST => 1, diff --git a/src/pocketmine/level/Level.php b/src/pocketmine/level/Level.php index a72206288..d40b5e604 100644 --- a/src/pocketmine/level/Level.php +++ b/src/pocketmine/level/Level.php @@ -387,16 +387,16 @@ class Level implements ChunkManager, Metadatable{ } public function registerGenerator(){ - $size = $this->server->getScheduler()->getAsyncTaskPoolSize(); - for($i = 0; $i < $size; ++$i){ - $this->server->getScheduler()->scheduleAsyncTaskToWorker(new GeneratorRegisterTask($this, $this->generatorInstance), $i); + $pool = $this->server->getAsyncPool(); + for($i = 0, $size = $pool->getSize(); $i < $size; ++$i){ + $pool->submitTaskToWorker(new GeneratorRegisterTask($this, $this->generatorInstance), $i); } } public function unregisterGenerator(){ - $size = $this->server->getScheduler()->getAsyncTaskPoolSize(); - for($i = 0; $i < $size; ++$i){ - $this->server->getScheduler()->scheduleAsyncTaskToWorker(new GeneratorUnregisterTask($this), $i); + $pool = $this->server->getAsyncPool(); + for($i = 0, $size = $pool->getSize(); $i < $size; ++$i){ + $pool->submitTaskToWorker(new GeneratorUnregisterTask($this), $i); } } @@ -2486,7 +2486,7 @@ class Level implements ChunkManager, Metadatable{ throw new ChunkException("Invalid Chunk sent"); } - $this->server->getScheduler()->scheduleAsyncTask(new ChunkRequestTask($this, $chunk)); + $this->server->getAsyncPool()->submitTask(new ChunkRequestTask($this, $chunk)); $this->timings->syncChunkSendPrepareTimer->stopTiming(); } @@ -2668,7 +2668,7 @@ class Level implements ChunkManager, Metadatable{ $this->server->getPluginManager()->callEvent(new ChunkLoadEvent($this, $chunk, !$chunk->isGenerated())); if(!$chunk->isLightPopulated() and $chunk->isPopulated() and $this->getServer()->getProperty("chunk-ticking.light-updates", false)){ - $this->getServer()->getScheduler()->scheduleAsyncTask(new LightPopulationTask($this, $chunk)); + $this->getServer()->getAsyncPool()->submitTask(new LightPopulationTask($this, $chunk)); } if($this->isChunkInUse($x, $z)){ @@ -2951,7 +2951,7 @@ class Level implements ChunkManager, Metadatable{ } } $task = new PopulationTask($this, $chunk); - $this->server->getScheduler()->scheduleAsyncTask($task); + $this->server->getAsyncPool()->submitTask($task); } } diff --git a/src/pocketmine/scheduler/AsyncPool.php b/src/pocketmine/scheduler/AsyncPool.php index 96f4e8cc6..4b136d4e6 100644 --- a/src/pocketmine/scheduler/AsyncPool.php +++ b/src/pocketmine/scheduler/AsyncPool.php @@ -24,7 +24,6 @@ declare(strict_types=1); namespace pocketmine\scheduler; use pocketmine\Server; -use pocketmine\timings\Timings; class AsyncPool{ @@ -37,6 +36,8 @@ class AsyncPool{ private $tasks = []; /** @var int[] */ private $taskWorkers = []; + /** @var int */ + private $nextTaskId = 1; /** @var AsyncWorker[] */ private $workers = []; @@ -77,13 +78,15 @@ class AsyncPool{ } public function submitTaskToWorker(AsyncTask $task, int $worker){ - if(isset($this->tasks[$task->getTaskId()]) or $task->isGarbage()){ - return; - } - if($worker < 0 or $worker >= $this->size){ throw new \InvalidArgumentException("Invalid worker $worker"); } + if($task->getTaskId() !== null){ + throw new \InvalidArgumentException("Cannot submit the same AsyncTask instance more than once"); + } + + $task->progressUpdates = new \Threaded; + $task->setTaskId($this->nextTaskId++); $this->tasks[$task->getTaskId()] = $task; @@ -93,8 +96,8 @@ class AsyncPool{ } public function submitTask(AsyncTask $task) : int{ - if(isset($this->tasks[$task->getTaskId()]) or $task->isGarbage()){ - return -1; + if($task->getTaskId() !== null){ + throw new \InvalidArgumentException("Cannot submit the same AsyncTask instance more than once"); } $selectedWorker = mt_rand(0, $this->size - 1); @@ -160,8 +163,6 @@ class AsyncPool{ } public function collectTasks(){ - Timings::$schedulerAsyncTimer->startTiming(); - foreach($this->tasks as $task){ if(!$task->isGarbage()){ $task->checkProgressUpdates($this->server); @@ -189,8 +190,6 @@ class AsyncPool{ } $this->collectWorkers(); - - Timings::$schedulerAsyncTimer->stopTiming(); } public function shutdown() : void{ diff --git a/src/pocketmine/scheduler/ServerScheduler.php b/src/pocketmine/scheduler/ServerScheduler.php index 210b9b736..a71111217 100644 --- a/src/pocketmine/scheduler/ServerScheduler.php +++ b/src/pocketmine/scheduler/ServerScheduler.php @@ -33,7 +33,6 @@ use pocketmine\Server; use pocketmine\utils\ReversePriorityQueue; class ServerScheduler{ - public static $WORKERS = 2; /** * @var ReversePriorityQueue */ @@ -44,9 +43,6 @@ class ServerScheduler{ */ protected $tasks = []; - /** @var AsyncPool */ - protected $asyncPool; - /** @var int */ private $ids = 1; @@ -55,7 +51,6 @@ class ServerScheduler{ public function __construct(){ $this->queue = new ReversePriorityQueue(); - $this->asyncPool = new AsyncPool(Server::getInstance(), self::$WORKERS); } /** @@ -67,49 +62,6 @@ class ServerScheduler{ return $this->addTask($task, -1, -1); } - /** - * Submits an asynchronous task to the Worker Pool - * - * @param AsyncTask $task - * - * @return int - */ - public function scheduleAsyncTask(AsyncTask $task) : int{ - if($task->getTaskId() !== null){ - throw new \UnexpectedValueException("Attempt to schedule the same AsyncTask instance twice"); - } - $id = $this->nextId(); - $task->setTaskId($id); - $task->progressUpdates = new \Threaded; - return $this->asyncPool->submitTask($task); - } - - /** - * Submits an asynchronous task to a specific Worker in the Pool - * - * @param AsyncTask $task - * @param int $worker - * - * @return void - */ - public function scheduleAsyncTaskToWorker(AsyncTask $task, int $worker){ - if($task->getTaskId() !== null){ - throw new \UnexpectedValueException("Attempt to schedule the same AsyncTask instance twice"); - } - $id = $this->nextId(); - $task->setTaskId($id); - $task->progressUpdates = new \Threaded; - $this->asyncPool->submitTaskToWorker($task, $worker); - } - - public function getAsyncTaskPoolSize() : int{ - return $this->asyncPool->getSize(); - } - - public function increaseAsyncTaskPoolSize(int $newSize){ - $this->asyncPool->increaseSize($newSize); - } - /** * @param Task $task * @param int $delay @@ -169,7 +121,6 @@ class ServerScheduler{ $task->cancel(); } $this->tasks = []; - $this->asyncPool->removeTasks(); while(!$this->queue->isEmpty()){ $this->queue->extract(); } @@ -228,7 +179,6 @@ class ServerScheduler{ public function shutdown() : void{ $this->cancelAllTasks(); - $this->asyncPool->shutdown(); } /** @@ -260,8 +210,6 @@ class ServerScheduler{ unset($this->tasks[$task->getTaskId()]); } } - - $this->asyncPool->collectTasks(); } private function isReady(int $currentTicks) : bool{ diff --git a/src/pocketmine/updater/AutoUpdater.php b/src/pocketmine/updater/AutoUpdater.php index a2a20e786..c581cc113 100644 --- a/src/pocketmine/updater/AutoUpdater.php +++ b/src/pocketmine/updater/AutoUpdater.php @@ -149,7 +149,7 @@ class AutoUpdater{ * Schedules an AsyncTask to check for an update. */ public function doCheck(){ - $this->server->getScheduler()->scheduleAsyncTask(new UpdateCheckTask($this->endpoint, $this->getChannel())); + $this->server->getAsyncPool()->submitTask(new UpdateCheckTask($this->endpoint, $this->getChannel())); } /** diff --git a/src/pocketmine/utils/Config.php b/src/pocketmine/utils/Config.php index 5a5b45617..c041b1d3b 100644 --- a/src/pocketmine/utils/Config.php +++ b/src/pocketmine/utils/Config.php @@ -217,7 +217,7 @@ class Config{ } if($async){ - Server::getInstance()->getScheduler()->scheduleAsyncTask(new FileWriteTask($this->file, $content)); + Server::getInstance()->getAsyncPool()->submitTask(new FileWriteTask($this->file, $content)); }else{ file_put_contents($this->file, $content); } diff --git a/tests/plugins/PocketMine-TesterPlugin b/tests/plugins/PocketMine-TesterPlugin index e60eba032..c54e6732b 160000 --- a/tests/plugins/PocketMine-TesterPlugin +++ b/tests/plugins/PocketMine-TesterPlugin @@ -1 +1 @@ -Subproject commit e60eba0324e4540abae317630ed758d3814e0137 +Subproject commit c54e6732b489b4124073eb6714c4fc594faf9c80 From 132746aa3d73d46f8d65ba40ea353b8b6f4172e0 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Wed, 30 May 2018 12:29:19 +0100 Subject: [PATCH 02/12] ServerScheduler: Require a Logger instance as ctor param, now non-dependent on Server yay for unit-testing and reusability!!! --- src/pocketmine/Server.php | 2 +- src/pocketmine/scheduler/ServerScheduler.php | 11 +++++++---- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/pocketmine/Server.php b/src/pocketmine/Server.php index f60675df0..92d5a1f46 100644 --- a/src/pocketmine/Server.php +++ b/src/pocketmine/Server.php @@ -1525,7 +1525,7 @@ class Server{ $this->logger->info($this->getLanguage()->translateString("pocketmine.server.start", [TextFormat::AQUA . $this->getVersion() . TextFormat::RESET])); - $this->scheduler = new ServerScheduler(); + $this->scheduler = new ServerScheduler($this->logger); if(($poolSize = $this->getProperty("settings.async-workers", "auto")) === "auto"){ $poolSize = 2; diff --git a/src/pocketmine/scheduler/ServerScheduler.php b/src/pocketmine/scheduler/ServerScheduler.php index a71111217..99b28c4d0 100644 --- a/src/pocketmine/scheduler/ServerScheduler.php +++ b/src/pocketmine/scheduler/ServerScheduler.php @@ -29,10 +29,12 @@ namespace pocketmine\scheduler; use pocketmine\plugin\Plugin; use pocketmine\plugin\PluginException; -use pocketmine\Server; use pocketmine\utils\ReversePriorityQueue; class ServerScheduler{ + /** @var \Logger */ + private $logger; + /** * @var ReversePriorityQueue */ @@ -49,7 +51,8 @@ class ServerScheduler{ /** @var int */ protected $currentTick = 0; - public function __construct(){ + public function __construct(\Logger $logger){ + $this->logger = $logger; $this->queue = new ReversePriorityQueue(); } @@ -197,8 +200,8 @@ class ServerScheduler{ try{ $task->run($this->currentTick); }catch(\Throwable $e){ - Server::getInstance()->getLogger()->critical("Could not execute task " . $task->getTaskName() . ": " . $e->getMessage()); - Server::getInstance()->getLogger()->logException($e); + $this->logger->critical("Could not execute task " . $task->getTaskName() . ": " . $e->getMessage()); + $this->logger->logException($e); } $task->timings->stopTiming(); } From 51f43fb375057dc18d75130b6c3071a12550587d Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Wed, 30 May 2018 14:11:11 +0100 Subject: [PATCH 03/12] Removed global ServerScheduler - plugins now get their own isolated schedulers This change breaks pretty much all API pertaining to synchronous task scheduling. Significant changes: - Server->getScheduler() has been removed - Plugin->getScheduler() has been added - every plugin now has its own scheduler - Because schedulers are now per-plugin, it is now unnecessary for PluginTask to exist because stopping plugin tasks on plugin disable is as simple as destroying the plugin's scheduler. Therefore PluginTask has now been removed and it is expected for things to now use the base Task class instead. For the most part, plugins will simply need to change Plugin->getServer()->getScheduler()->... to Plugin->getScheduler()->... Another highlight is that plugin tasks now no longer have global IDs - they are unique to each scheduler. --- src/pocketmine/Server.php | 18 +------ src/pocketmine/plugin/Plugin.php | 6 +++ src/pocketmine/plugin/PluginBase.php | 11 ++++ src/pocketmine/plugin/PluginManager.php | 10 +++- src/pocketmine/scheduler/PluginTask.php | 50 ------------------- src/pocketmine/scheduler/TaskHandler.php | 30 ++++++----- ...{ServerScheduler.php => TaskScheduler.php} | 36 ++++++------- src/pocketmine/timings/Timings.php | 16 +----- tests/plugins/PocketMine-TesterPlugin | 2 +- 9 files changed, 61 insertions(+), 118 deletions(-) delete mode 100644 src/pocketmine/scheduler/PluginTask.php rename src/pocketmine/scheduler/{ServerScheduler.php => TaskScheduler.php} (85%) diff --git a/src/pocketmine/Server.php b/src/pocketmine/Server.php index 92d5a1f46..56a6de309 100644 --- a/src/pocketmine/Server.php +++ b/src/pocketmine/Server.php @@ -90,7 +90,6 @@ use pocketmine\resourcepacks\ResourcePackManager; use pocketmine\scheduler\AsyncPool; use pocketmine\scheduler\FileWriteTask; use pocketmine\scheduler\SendUsageTask; -use pocketmine\scheduler\ServerScheduler; use pocketmine\snooze\SleeperHandler; use pocketmine\snooze\SleeperNotifier; use pocketmine\tile\Tile; @@ -149,8 +148,6 @@ class Server{ /** @var AutoUpdater */ private $updater = null; - /** @var ServerScheduler */ - private $scheduler = null; /** @var AsyncPool */ private $asyncPool; @@ -653,13 +650,6 @@ class Server{ return $this->resourceManager; } - /** - * @return ServerScheduler - */ - public function getScheduler(){ - return $this->scheduler; - } - public function getAsyncPool() : AsyncPool{ return $this->asyncPool; } @@ -1525,8 +1515,6 @@ class Server{ $this->logger->info($this->getLanguage()->translateString("pocketmine.server.start", [TextFormat::AQUA . $this->getVersion() . TextFormat::RESET])); - $this->scheduler = new ServerScheduler($this->logger); - if(($poolSize = $this->getProperty("settings.async-workers", "auto")) === "auto"){ $poolSize = 2; $processors = Utils::getCoreCount() - 2; @@ -2075,10 +2063,6 @@ class Server{ $this->getLogger()->debug("Removing event handlers"); HandlerList::unregisterAll(); - if($this->scheduler instanceof ServerScheduler){ - $this->getLogger()->debug("Stopping all tasks"); - $this->scheduler->cancelAllTasks(); - } if($this->asyncPool instanceof AsyncPool){ $this->getLogger()->debug("Shutting down async task worker pool"); $this->asyncPool->shutdown(); @@ -2534,7 +2518,7 @@ class Server{ Timings::$connectionTimer->stopTiming(); Timings::$schedulerTimer->startTiming(); - $this->scheduler->mainThreadHeartbeat($this->tickCounter); + $this->pluginManager->tickSchedulers($this->tickCounter); Timings::$schedulerTimer->stopTiming(); Timings::$schedulerAsyncTimer->startTiming(); diff --git a/src/pocketmine/plugin/Plugin.php b/src/pocketmine/plugin/Plugin.php index ecb008ac7..867b9a046 100644 --- a/src/pocketmine/plugin/Plugin.php +++ b/src/pocketmine/plugin/Plugin.php @@ -27,6 +27,7 @@ declare(strict_types=1); namespace pocketmine\plugin; use pocketmine\command\CommandExecutor; +use pocketmine\scheduler\TaskScheduler; use pocketmine\Server; use pocketmine\utils\Config; @@ -136,4 +137,9 @@ interface Plugin extends CommandExecutor{ */ public function getPluginLoader(); + /** + * @return TaskScheduler + */ + public function getScheduler() : TaskScheduler; + } diff --git a/src/pocketmine/plugin/PluginBase.php b/src/pocketmine/plugin/PluginBase.php index 6f7b4f6fe..de66a85d2 100644 --- a/src/pocketmine/plugin/PluginBase.php +++ b/src/pocketmine/plugin/PluginBase.php @@ -26,6 +26,7 @@ namespace pocketmine\plugin; use pocketmine\command\Command; use pocketmine\command\CommandSender; use pocketmine\command\PluginIdentifiableCommand; +use pocketmine\scheduler\TaskScheduler; use pocketmine\Server; use pocketmine\utils\Config; @@ -58,6 +59,9 @@ abstract class PluginBase implements Plugin{ /** @var PluginLogger */ private $logger; + /** @var TaskScheduler */ + private $scheduler; + /** * Called when the plugin is loaded, before calling onEnable() */ @@ -119,6 +123,7 @@ abstract class PluginBase implements Plugin{ $this->file = rtrim($file, "\\/") . "/"; $this->configFile = $this->dataFolder . "config.yml"; $this->logger = new PluginLogger($this); + $this->scheduler = new TaskScheduler($this->logger); } } @@ -305,4 +310,10 @@ abstract class PluginBase implements Plugin{ return $this->loader; } + /** + * @return TaskScheduler + */ + public function getScheduler() : TaskScheduler{ + return $this->scheduler; + } } diff --git a/src/pocketmine/plugin/PluginManager.php b/src/pocketmine/plugin/PluginManager.php index 6ea2dc603..f7dfb1d49 100644 --- a/src/pocketmine/plugin/PluginManager.php +++ b/src/pocketmine/plugin/PluginManager.php @@ -657,7 +657,7 @@ class PluginManager{ $this->server->getLogger()->logException($e); } - $this->server->getScheduler()->cancelTasks($plugin); + $plugin->getScheduler()->shutdown(); HandlerList::unregisterAll($plugin); foreach($plugin->getDescription()->getPermissions() as $perm){ $this->removePermission($perm); @@ -665,6 +665,14 @@ class PluginManager{ } } + public function tickSchedulers(int $currentTick) : void{ + foreach($this->plugins as $p){ + if($p->isEnabled()){ + $p->getScheduler()->mainThreadHeartbeat($currentTick); + } + } + } + public function clearPlugins(){ $this->disablePlugins(); $this->plugins = []; diff --git a/src/pocketmine/scheduler/PluginTask.php b/src/pocketmine/scheduler/PluginTask.php deleted file mode 100644 index c5d503ec4..000000000 --- a/src/pocketmine/scheduler/PluginTask.php +++ /dev/null @@ -1,50 +0,0 @@ -owner = $owner; - } - - /** - * @return Plugin - */ - final public function getOwner() : Plugin{ - return $this->owner; - } - -} diff --git a/src/pocketmine/scheduler/TaskHandler.php b/src/pocketmine/scheduler/TaskHandler.php index 0e980b96f..6af785230 100644 --- a/src/pocketmine/scheduler/TaskHandler.php +++ b/src/pocketmine/scheduler/TaskHandler.php @@ -50,22 +50,26 @@ class TaskHandler{ /** @var TimingsHandler */ public $timings; - public $timingName = null; + /** @var string */ + private $taskName; + /** @var string */ + private $ownerName; /** - * @param string $timingName - * @param Task $task - * @param int $taskId - * @param int $delay - * @param int $period + * @param Task $task + * @param int $taskId + * @param int $delay + * @param int $period + * @param string|null $ownerName */ - public function __construct(string $timingName, Task $task, int $taskId, int $delay = -1, int $period = -1){ + public function __construct(Task $task, int $taskId, int $delay = -1, int $period = -1, ?string $ownerName = null){ $this->task = $task; $this->taskId = $taskId; $this->delay = $delay; $this->period = $period; - $this->timingName = $timingName ?? "Unknown"; - $this->timings = Timings::getPluginTaskTimings($this, $period); + $this->taskName = get_class($task); + $this->ownerName = $ownerName ?? "Unknown"; + $this->timings = Timings::getScheduledTaskTimings($this, $period); $this->task->setHandler($this); } @@ -164,10 +168,10 @@ class TaskHandler{ * @return string */ public function getTaskName() : string{ - if($this->timingName !== null){ - return $this->timingName; - } + return $this->taskName; + } - return get_class($this->task); + public function getOwnerName() : string{ + return $this->ownerName; } } diff --git a/src/pocketmine/scheduler/ServerScheduler.php b/src/pocketmine/scheduler/TaskScheduler.php similarity index 85% rename from src/pocketmine/scheduler/ServerScheduler.php rename to src/pocketmine/scheduler/TaskScheduler.php index 99b28c4d0..f24d900c6 100644 --- a/src/pocketmine/scheduler/ServerScheduler.php +++ b/src/pocketmine/scheduler/TaskScheduler.php @@ -27,13 +27,16 @@ declare(strict_types=1); namespace pocketmine\scheduler; -use pocketmine\plugin\Plugin; -use pocketmine\plugin\PluginException; use pocketmine\utils\ReversePriorityQueue; -class ServerScheduler{ +class TaskScheduler{ /** @var \Logger */ private $logger; + /** @var string|null */ + private $owner; + + /** @var bool */ + private $shutdown = false; /** * @var ReversePriorityQueue @@ -51,8 +54,10 @@ class ServerScheduler{ /** @var int */ protected $currentTick = 0; - public function __construct(\Logger $logger){ + + public function __construct(\Logger $logger, ?string $owner = null){ $this->logger = $logger; + $this->owner = $owner; $this->queue = new ReversePriorityQueue(); } @@ -106,19 +111,6 @@ class ServerScheduler{ } } - /** - * @param Plugin $plugin - */ - public function cancelTasks(Plugin $plugin){ - foreach($this->tasks as $taskId => $task){ - $ptask = $task->getTask(); - if($ptask instanceof PluginTask and $ptask->getOwner() === $plugin){ - $task->cancel(); - unset($this->tasks[$taskId]); - } - } - } - public function cancelAllTasks(){ foreach($this->tasks as $task){ $task->cancel(); @@ -146,11 +138,11 @@ class ServerScheduler{ * * @return null|TaskHandler * - * @throws PluginException + * @throws \InvalidStateException */ private function addTask(Task $task, int $delay, int $period){ - if($task instanceof PluginTask and !$task->getOwner()->isEnabled()){ - throw new PluginException("Plugin '" . $task->getOwner()->getName() . "' attempted to register a task while disabled"); + if($this->shutdown){ + throw new \InvalidStateException("Tried to schedule task after scheduler shutdown"); } if($delay <= 0){ @@ -163,7 +155,7 @@ class ServerScheduler{ $period = 1; } - return $this->handle(new TaskHandler(get_class($task), $task, $this->nextId(), $delay, $period)); + return $this->handle(new TaskHandler($task, $this->nextId(), $delay, $period, $this->owner)); } private function handle(TaskHandler $handler) : TaskHandler{ @@ -181,6 +173,7 @@ class ServerScheduler{ } public function shutdown() : void{ + $this->shutdown = true; $this->cancelAllTasks(); } @@ -225,5 +218,4 @@ class ServerScheduler{ private function nextId() : int{ return $this->ids++; } - } diff --git a/src/pocketmine/timings/Timings.php b/src/pocketmine/timings/Timings.php index cc92c6839..3e9145fab 100644 --- a/src/pocketmine/timings/Timings.php +++ b/src/pocketmine/timings/Timings.php @@ -27,7 +27,6 @@ use pocketmine\entity\Entity; use pocketmine\network\mcpe\protocol\DataPacket; use pocketmine\Player; use pocketmine\plugin\PluginManager; -use pocketmine\scheduler\PluginTask; use pocketmine\scheduler\TaskHandler; use pocketmine\tile\Tile; @@ -149,19 +148,8 @@ abstract class Timings{ * * @return TimingsHandler */ - public static function getPluginTaskTimings(TaskHandler $task, int $period) : TimingsHandler{ - $ftask = $task->getTask(); - if($ftask instanceof PluginTask and $ftask->getOwner() !== null){ - $plugin = $ftask->getOwner()->getDescription()->getFullName(); - }elseif($task->timingName !== null){ - $plugin = "Scheduler"; - }else{ - $plugin = "Unknown"; - } - - $taskname = $task->getTaskName(); - - $name = "Task: " . $plugin . " Runnable: " . $taskname; + public static function getScheduledTaskTimings(TaskHandler $task, int $period) : TimingsHandler{ + $name = "Task: " . ($task->getOwnerName() ?? "Unknown") . " Runnable: " . $task->getTaskName(); if($period > 0){ $name .= "(interval:" . $period . ")"; diff --git a/tests/plugins/PocketMine-TesterPlugin b/tests/plugins/PocketMine-TesterPlugin index c54e6732b..8906ba983 160000 --- a/tests/plugins/PocketMine-TesterPlugin +++ b/tests/plugins/PocketMine-TesterPlugin @@ -1 +1 @@ -Subproject commit c54e6732b489b4124073eb6714c4fc594faf9c80 +Subproject commit 8906ba9831c19cc36d66bb90c43ee1f9d674b0b2 From e20be3eeba57c7c60ef0db2957a57829d60343e0 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Wed, 30 May 2018 15:38:39 +0100 Subject: [PATCH 04/12] Move task timings responsibility from scheduler to handler --- src/pocketmine/scheduler/TaskHandler.php | 9 +++++++-- src/pocketmine/scheduler/TaskScheduler.php | 2 -- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/pocketmine/scheduler/TaskHandler.php b/src/pocketmine/scheduler/TaskHandler.php index 6af785230..4b83b2e44 100644 --- a/src/pocketmine/scheduler/TaskHandler.php +++ b/src/pocketmine/scheduler/TaskHandler.php @@ -48,7 +48,7 @@ class TaskHandler{ protected $cancelled = false; /** @var TimingsHandler */ - public $timings; + private $timings; /** @var string */ private $taskName; @@ -161,7 +161,12 @@ class TaskHandler{ * @param int $currentTick */ public function run(int $currentTick){ - $this->task->onRun($currentTick); + $this->timings->startTiming(); + try{ + $this->task->onRun($currentTick); + }finally{ + $this->timings->stopTiming(); + } } /** diff --git a/src/pocketmine/scheduler/TaskScheduler.php b/src/pocketmine/scheduler/TaskScheduler.php index f24d900c6..294ddd3a9 100644 --- a/src/pocketmine/scheduler/TaskScheduler.php +++ b/src/pocketmine/scheduler/TaskScheduler.php @@ -189,14 +189,12 @@ class TaskScheduler{ unset($this->tasks[$task->getTaskId()]); continue; }else{ - $task->timings->startTiming(); try{ $task->run($this->currentTick); }catch(\Throwable $e){ $this->logger->critical("Could not execute task " . $task->getTaskName() . ": " . $e->getMessage()); $this->logger->logException($e); } - $task->timings->stopTiming(); } if($task->isRepeating()){ $task->setNextRun($this->currentTick + $task->getPeriod()); From a8c766be88196b8ec6507439b958b5ec081574f3 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Wed, 30 May 2018 16:00:25 +0100 Subject: [PATCH 05/12] Remove TaskHandler dependency on MainLogger This instead allows the exception to be caught by the scheduler and reported using its logger. --- src/pocketmine/scheduler/TaskHandler.php | 3 --- src/pocketmine/scheduler/TaskScheduler.php | 11 ++++++++--- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/pocketmine/scheduler/TaskHandler.php b/src/pocketmine/scheduler/TaskHandler.php index 4b83b2e44..5c86d3d46 100644 --- a/src/pocketmine/scheduler/TaskHandler.php +++ b/src/pocketmine/scheduler/TaskHandler.php @@ -25,7 +25,6 @@ namespace pocketmine\scheduler; use pocketmine\timings\Timings; use pocketmine\timings\TimingsHandler; -use pocketmine\utils\MainLogger; class TaskHandler{ @@ -145,8 +144,6 @@ class TaskHandler{ if(!$this->isCancelled()){ $this->task->onCancel(); } - }catch(\Throwable $e){ - MainLogger::getLogger()->logException($e); }finally{ $this->remove(); } diff --git a/src/pocketmine/scheduler/TaskScheduler.php b/src/pocketmine/scheduler/TaskScheduler.php index 294ddd3a9..1c7af2e97 100644 --- a/src/pocketmine/scheduler/TaskScheduler.php +++ b/src/pocketmine/scheduler/TaskScheduler.php @@ -106,14 +106,19 @@ class TaskScheduler{ */ public function cancelTask(int $taskId){ if($taskId !== null and isset($this->tasks[$taskId])){ - $this->tasks[$taskId]->cancel(); + try{ + $this->tasks[$taskId]->cancel(); + }catch(\Throwable $e){ + $this->logger->critical("Task " . $this->tasks[$taskId]->getTaskName() . " threw an exception when trying to cancel: " . $e->getMessage()); + $this->logger->logException($e); + } unset($this->tasks[$taskId]); } } public function cancelAllTasks(){ - foreach($this->tasks as $task){ - $task->cancel(); + foreach($this->tasks as $id => $task){ + $this->cancelTask($id); } $this->tasks = []; while(!$this->queue->isEmpty()){ From 18fdbc2834572007e839463fdcc274d7026304e5 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Wed, 30 May 2018 16:45:37 +0100 Subject: [PATCH 06/12] TaskScheduler: fix typo in isReady() parameter --- src/pocketmine/scheduler/TaskScheduler.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/pocketmine/scheduler/TaskScheduler.php b/src/pocketmine/scheduler/TaskScheduler.php index 1c7af2e97..9cec744e8 100644 --- a/src/pocketmine/scheduler/TaskScheduler.php +++ b/src/pocketmine/scheduler/TaskScheduler.php @@ -211,8 +211,8 @@ class TaskScheduler{ } } - private function isReady(int $currentTicks) : bool{ - return count($this->tasks) > 0 and $this->queue->current()->getNextRun() <= $currentTicks; + private function isReady(int $currentTick) : bool{ + return count($this->tasks) > 0 and $this->queue->current()->getNextRun() <= $currentTick; } /** From b3043f95527adbaae999f5fc6627a8bc49c55116 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Wed, 30 May 2018 21:30:43 +0100 Subject: [PATCH 07/12] Task: remove obsolete doc comment not anymore sunshine --- src/pocketmine/scheduler/Task.php | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/pocketmine/scheduler/Task.php b/src/pocketmine/scheduler/Task.php index 7575352f2..fda37d428 100644 --- a/src/pocketmine/scheduler/Task.php +++ b/src/pocketmine/scheduler/Task.php @@ -23,9 +23,6 @@ declare(strict_types=1); namespace pocketmine\scheduler; -/** - * WARNING! Tasks created by plugins MUST extend PluginTask - */ abstract class Task{ /** @var TaskHandler */ From 15270f832993d7d8b5a51589f3f978ffe546907a Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Thu, 31 May 2018 12:34:22 +0100 Subject: [PATCH 08/12] Fixed plugin schedulers crashing after disable/reenable --- src/pocketmine/plugin/PluginManager.php | 1 + src/pocketmine/scheduler/TaskScheduler.php | 10 +++++++--- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/src/pocketmine/plugin/PluginManager.php b/src/pocketmine/plugin/PluginManager.php index f7dfb1d49..7244c4113 100644 --- a/src/pocketmine/plugin/PluginManager.php +++ b/src/pocketmine/plugin/PluginManager.php @@ -575,6 +575,7 @@ class PluginManager{ foreach($plugin->getDescription()->getPermissions() as $perm){ $this->addPermission($perm); } + $plugin->getScheduler()->setEnabled(true); $plugin->getPluginLoader()->enablePlugin($plugin); }catch(\Throwable $e){ $this->server->getLogger()->logException($e); diff --git a/src/pocketmine/scheduler/TaskScheduler.php b/src/pocketmine/scheduler/TaskScheduler.php index 9cec744e8..77c37fa1b 100644 --- a/src/pocketmine/scheduler/TaskScheduler.php +++ b/src/pocketmine/scheduler/TaskScheduler.php @@ -36,7 +36,7 @@ class TaskScheduler{ private $owner; /** @var bool */ - private $shutdown = false; + private $enabled = true; /** * @var ReversePriorityQueue @@ -146,7 +146,7 @@ class TaskScheduler{ * @throws \InvalidStateException */ private function addTask(Task $task, int $delay, int $period){ - if($this->shutdown){ + if(!$this->enabled){ throw new \InvalidStateException("Tried to schedule task after scheduler shutdown"); } @@ -178,10 +178,14 @@ class TaskScheduler{ } public function shutdown() : void{ - $this->shutdown = true; + $this->enabled = false; $this->cancelAllTasks(); } + public function setEnabled(bool $enabled) : void{ + $this->enabled = $enabled; + } + /** * @param int $currentTick */ From 60212cef2fc04c3359c5248d0d5cf618336bc3e9 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Thu, 31 May 2018 12:38:36 +0100 Subject: [PATCH 09/12] TaskScheduler: adjust disabled scheduler exception --- src/pocketmine/scheduler/TaskScheduler.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pocketmine/scheduler/TaskScheduler.php b/src/pocketmine/scheduler/TaskScheduler.php index 77c37fa1b..7f93ef462 100644 --- a/src/pocketmine/scheduler/TaskScheduler.php +++ b/src/pocketmine/scheduler/TaskScheduler.php @@ -147,7 +147,7 @@ class TaskScheduler{ */ private function addTask(Task $task, int $delay, int $period){ if(!$this->enabled){ - throw new \InvalidStateException("Tried to schedule task after scheduler shutdown"); + throw new \InvalidStateException("Tried to schedule task to disabled scheduler"); } if($delay <= 0){ From f2b8d6879febb9c09a06dea288e0733b2259151d Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Thu, 31 May 2018 12:52:29 +0100 Subject: [PATCH 10/12] TaskScheduler: remove leftover code that makes no sense this is impossible now that we have typehints --- src/pocketmine/scheduler/TaskScheduler.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pocketmine/scheduler/TaskScheduler.php b/src/pocketmine/scheduler/TaskScheduler.php index 7f93ef462..d756ca15d 100644 --- a/src/pocketmine/scheduler/TaskScheduler.php +++ b/src/pocketmine/scheduler/TaskScheduler.php @@ -105,7 +105,7 @@ class TaskScheduler{ * @param int $taskId */ public function cancelTask(int $taskId){ - if($taskId !== null and isset($this->tasks[$taskId])){ + if(isset($this->tasks[$taskId])){ try{ $this->tasks[$taskId]->cancel(); }catch(\Throwable $e){ From 6b4b4e4bb1e86001884123cafcd6f41fe8901718 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Thu, 31 May 2018 13:01:06 +0100 Subject: [PATCH 11/12] TaskScheduler: remove redundant else branch --- src/pocketmine/scheduler/TaskScheduler.php | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/pocketmine/scheduler/TaskScheduler.php b/src/pocketmine/scheduler/TaskScheduler.php index d756ca15d..618bb0110 100644 --- a/src/pocketmine/scheduler/TaskScheduler.php +++ b/src/pocketmine/scheduler/TaskScheduler.php @@ -197,13 +197,12 @@ class TaskScheduler{ if($task->isCancelled()){ unset($this->tasks[$task->getTaskId()]); continue; - }else{ - try{ - $task->run($this->currentTick); - }catch(\Throwable $e){ - $this->logger->critical("Could not execute task " . $task->getTaskName() . ": " . $e->getMessage()); - $this->logger->logException($e); - } + } + try{ + $task->run($this->currentTick); + }catch(\Throwable $e){ + $this->logger->critical("Could not execute task " . $task->getTaskName() . ": " . $e->getMessage()); + $this->logger->logException($e); } if($task->isRepeating()){ $task->setNextRun($this->currentTick + $task->getPeriod()); From 02b4eeeb9b0b7ff99da6100a8f72979d1e3ef161 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Thu, 31 May 2018 14:10:59 +0100 Subject: [PATCH 12/12] TaskScheduler: Remove repeating tasks which throw exceptions --- src/pocketmine/scheduler/TaskScheduler.php | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/src/pocketmine/scheduler/TaskScheduler.php b/src/pocketmine/scheduler/TaskScheduler.php index 618bb0110..a641aa292 100644 --- a/src/pocketmine/scheduler/TaskScheduler.php +++ b/src/pocketmine/scheduler/TaskScheduler.php @@ -198,19 +198,26 @@ class TaskScheduler{ unset($this->tasks[$task->getTaskId()]); continue; } + $crashed = false; try{ $task->run($this->currentTick); }catch(\Throwable $e){ + $crashed = true; $this->logger->critical("Could not execute task " . $task->getTaskName() . ": " . $e->getMessage()); $this->logger->logException($e); } if($task->isRepeating()){ - $task->setNextRun($this->currentTick + $task->getPeriod()); - $this->queue->insert($task, $this->currentTick + $task->getPeriod()); - }else{ - $task->remove(); - unset($this->tasks[$task->getTaskId()]); + if($crashed){ + $this->logger->debug("Dropping repeating task " . $task->getTaskName() . " due to exceptions thrown while running"); + }else{ + $task->setNextRun($this->currentTick + $task->getPeriod()); + $this->queue->insert($task, $this->currentTick + $task->getPeriod()); + continue; + } } + + $task->remove(); + unset($this->tasks[$task->getTaskId()]); } }