From 53f95c99cbf141633c8374f846d993cf200b4da8 Mon Sep 17 00:00:00 2001 From: "Eloy Lafuente (stronk7)" Date: Wed, 11 May 2016 23:57:54 +0200 Subject: [PATCH] MDL-54205 backup: loggers close() and destroy() Any backup & restore operation may be leaving opened files if a file logger is being used. This implementes the close() method, so every logger can close any resource. Also, the recommended backup_controlled::destroy() method now calls to new logger::destroy() method in charge of deleting all the references and closing any resource. Finally, some internally used controllers, were missing their destroy call, leading to associated loggers to remain open. Now all them are explicitly deltroyed. --- backup/controller/backup_controller.class.php | 4 +++- .../controller/restore_controller.class.php | 4 +++- backup/upgrade.txt | 8 +++++++ backup/util/dbops/restore_dbops.class.php | 10 ++++++-- backup/util/helper/backup_helper.class.php | 1 + backup/util/loggers/base_logger.class.php | 24 +++++++++++++++++++ backup/util/loggers/file_logger.class.php | 13 ++++++++++ 7 files changed, 60 insertions(+), 4 deletions(-) diff --git a/backup/controller/backup_controller.class.php b/backup/controller/backup_controller.class.php index e270f9e1f86..35edd9c318a 100644 --- a/backup/controller/backup_controller.class.php +++ b/backup/controller/backup_controller.class.php @@ -158,6 +158,8 @@ class backup_controller extends base_controller { public function destroy() { // Only need to destroy circulars under the plan. Delegate to it. $this->plan->destroy(); + // Loggers may have also chained references, destroy them. Also closing resources when needed. + $this->logger->destroy(); } public function finish_ui() { @@ -184,7 +186,7 @@ class backup_controller extends base_controller { $this->save_controller(); $tbc = self::load_controller($this->backupid); $this->logger = $tbc->logger; // wakeup loggers - $tbc->destroy(); // Clean temp controller structures + $tbc->plan->destroy(); // Clean plan controller structures, keeping logger alive. } else if ($status == backup::STATUS_FINISHED_OK) { // If the operation has ended without error (backup::STATUS_FINISHED_OK) diff --git a/backup/controller/restore_controller.class.php b/backup/controller/restore_controller.class.php index 8c72c588359..2829b998766 100644 --- a/backup/controller/restore_controller.class.php +++ b/backup/controller/restore_controller.class.php @@ -170,6 +170,8 @@ class restore_controller extends base_controller { public function destroy() { // Only need to destroy circulars under the plan. Delegate to it. $this->plan->destroy(); + // Loggers may have also chained references, destroy them. Also closing resources when needed. + $this->logger->destroy(); } public function finish_ui() { @@ -196,7 +198,7 @@ class restore_controller extends base_controller { $this->save_controller(); $tbc = self::load_controller($this->restoreid); $this->logger = $tbc->logger; // wakeup loggers - $tbc->destroy(); // Clean temp controller structures + $tbc->plan->destroy(); // Clean plan controller structures, keeping logger alive. } else if ($status == backup::STATUS_FINISHED_OK) { // If the operation has ended without error (backup::STATUS_FINISHED_OK) diff --git a/backup/upgrade.txt b/backup/upgrade.txt index 31e7f031106..88a9c885691 100644 --- a/backup/upgrade.txt +++ b/backup/upgrade.txt @@ -1,6 +1,14 @@ This files describes API changes in /backup/*, information provided here is intended especially for developers. +=== 3.1 === + +* New close() method added to loggers so they can close any open resource. Previously + any backup and restore operation using the file logger may be leaving unclosed files. +* New destroy() method added to loggers, normally called from backup and restore controllers + own destroy() method to ensure that all references in the chained loggers are deleted + and any open resource within them is closed properly. + === 3.0 === * The backup_auto_keep setting, in automated backups configuration, is now diff --git a/backup/util/dbops/restore_dbops.class.php b/backup/util/dbops/restore_dbops.class.php index 120ea6c7d4e..6d48d36ea9c 100644 --- a/backup/util/dbops/restore_dbops.class.php +++ b/backup/util/dbops/restore_dbops.class.php @@ -101,9 +101,11 @@ abstract class restore_dbops { // If included, add it if ($included) { - $includedtasks[] = $task; + $includedtasks[] = clone($task); // A clone is enough. In fact we only need the basepath. } } + $rc->destroy(); // Always need to destroy. + return $includedtasks; } @@ -1510,8 +1512,12 @@ abstract class restore_dbops { // Calculate the context we are going to use for capability checking $context = context_course::instance($courseid); + // TODO: Some day we must kill this dependency and change the process + // to pass info around without loading a controller copy. // When conflicting users are detected we may need original site info. - $restoreinfo = restore_controller_dbops::load_controller($restoreid)->get_info(); + $rc = restore_controller_dbops::load_controller($restoreid); + $restoreinfo = $rc->get_info(); + $rc->destroy(); // Always need to destroy. // Calculate if we have perms to create users, by checking: // to 'moodle/restore:createuser' and 'moodle/restore:userinfo' diff --git a/backup/util/helper/backup_helper.class.php b/backup/util/helper/backup_helper.class.php index 4096371b765..a5a7faa0fb5 100644 --- a/backup/util/helper/backup_helper.class.php +++ b/backup/util/helper/backup_helper.class.php @@ -302,6 +302,7 @@ abstract class backup_helper { $bc = backup_controller::load_controller($backupid); $bc->log('Attempt to copy backup file to the specified directory using filesystem failed - ', backup::LOG_WARNING, $dir); + $bc->destroy(); } // bad luck, try to deal with the file the old way - keep backup in file area if we can not copy to ext system } diff --git a/backup/util/loggers/base_logger.class.php b/backup/util/loggers/base_logger.class.php index ed2125cf37e..32b0c06c939 100644 --- a/backup/util/loggers/base_logger.class.php +++ b/backup/util/loggers/base_logger.class.php @@ -71,6 +71,30 @@ abstract class base_logger implements checksumable { return $this->level; } + /** + * Destroy (nullify) the chain of loggers references, also closing resources when needed. + * + * @since Moodle 3.1 + */ + public final function destroy() { + // Recursively destroy the chain. + if ($this->next !== null) { + $this->next->destroy(); + $this->next = null; + } + // And close every logger. + $this->close(); + } + + /** + * Close any resource the logger may have open. + * + * @since Moodle 3.1 + */ + public function close() { + // Nothing to do by default. Only loggers using resources (files, own connections...) need to override this. + } + // checksumable interface methods public function calculate_checksum() { diff --git a/backup/util/loggers/file_logger.class.php b/backup/util/loggers/file_logger.class.php index 5c05380d36f..98ff74acf39 100644 --- a/backup/util/loggers/file_logger.class.php +++ b/backup/util/loggers/file_logger.class.php @@ -66,6 +66,19 @@ class file_logger extends base_logger { } } + /** + * Close the logger resources (file handle) if still open. + * + * @since Moodle 3.1 + */ + public function close() { + // Close the file handle if hasn't been closed already. + if (is_resource($this->fhandle)) { + fclose($this->fhandle); + $this->fhandle = null; + } + } + // Protected API starts here protected function action($message, $level, $options = null) { -- 2.39.2