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) {