More careful destructors.

src/Backups/BackupIO_Disk.cpp

@@ -1,9 +1,7 @@
 #include <Backups/BackupIO_Disk.h>
-#include <Common/Exception.h>
 #include <Disks/IDisk.h>
 #include <IO/ReadBufferFromFileBase.h>
 #include <IO/WriteBufferFromFileBase.h>
-#include <Common/logger_useful.h>
 
 
 namespace DB
@@ -49,17 +47,10 @@ std::unique_ptr<WriteBuffer> BackupWriterDisk::writeFile(const String & file_nam
 
 void BackupWriterDisk::removeFilesAfterFailure(const Strings & file_names)
 {
-    try
+    for (const auto & file_name : file_names)
-    {
+        disk->removeFileIfExists(path / file_name);
-        for (const auto & file_name : file_names)
+    if (disk->isDirectory(path) && disk->isDirectoryEmpty(path))
-            disk->removeFileIfExists(path / file_name);
+        disk->removeDirectory(path);
-        if (disk->isDirectory(path) && disk->isDirectoryEmpty(path))
-            disk->removeDirectory(path);
-    }
-    catch (...)
-    {
-        LOG_WARNING(&Poco::Logger::get("BackupWriterDisk"), "RemoveFilesAfterFailure: {}", getCurrentExceptionMessage(false));
-    }
 }
 
 }

src/Backups/BackupIO_File.cpp

@@ -1,8 +1,6 @@
 #include <Backups/BackupIO_File.h>
-#include <Common/Exception.h>
 #include <Disks/IO/createReadBufferFromFileBase.h>
 #include <IO/WriteBufferFromFile.h>
-#include <Common/logger_useful.h>
 
 namespace fs = std::filesystem;
 
@@ -50,17 +48,10 @@ std::unique_ptr<WriteBuffer> BackupWriterFile::writeFile(const String & file_nam
 
 void BackupWriterFile::removeFilesAfterFailure(const Strings & file_names)
 {
-    try
+    for (const auto & file_name : file_names)
-    {
+        fs::remove(path / file_name);
-        for (const auto & file_name : file_names)
+    if (fs::is_directory(path) && fs::is_empty(path))
-            fs::remove(path / file_name);
+        fs::remove(path);
-        if (fs::is_directory(path) && fs::is_empty(path))
-            fs::remove(path);
-    }
-    catch (...)
-    {
-        LOG_WARNING(&Poco::Logger::get("BackupWriterFile"), "RemoveFilesAfterFailure: {}", getCurrentExceptionMessage(false));
-    }
 }
 
 }

src/Backups/BackupImpl.cpp

@@ -167,7 +167,14 @@ BackupImpl::BackupImpl(
 
 BackupImpl::~BackupImpl()
 {
-    close();
+    try
+    {
+        close();
+    }
+    catch (...)
+    {
+        DB::tryLogCurrentException(__PRETTY_FUNCTION__);
+    }
 }
 
 
@@ -231,10 +238,11 @@ void BackupImpl::close()
         archive_writer = {"", nullptr};
 
     if (!is_internal_backup && writer && !writing_finalized)
-    {
-        LOG_INFO(log, "Removing all files of backup {} after failure", backup_name);
         removeAllFilesAfterFailure();
-    }
+
+    writer.reset();
+    reader.reset();
+    coordination.reset();
 }
 
 time_t BackupImpl::getTimestamp() const
@@ -733,24 +741,33 @@ std::shared_ptr<IArchiveWriter> BackupImpl::getArchiveWriter(const String & suff
 
 void BackupImpl::removeAllFilesAfterFailure()
 {
-    Strings files_to_remove;
+    try
-    if (use_archives)
     {
-        files_to_remove.push_back(archive_params.archive_name);
+        LOG_INFO(log, "Removing all files of backup {} after failure", backup_name);
-        for (const auto & suffix : coordination->getAllArchiveSuffixes())
+
+        Strings files_to_remove;
+        if (use_archives)
         {
-            String archive_name_with_suffix = getArchiveNameWithSuffix(suffix);
+            files_to_remove.push_back(archive_params.archive_name);
-            files_to_remove.push_back(std::move(archive_name_with_suffix));
+            for (const auto & suffix : coordination->getAllArchiveSuffixes())
+            {
+                String archive_name_with_suffix = getArchiveNameWithSuffix(suffix);
+                files_to_remove.push_back(std::move(archive_name_with_suffix));
+            }
         }
+        else
+        {
+            files_to_remove.push_back(".backup");
+            for (const auto & file_info : coordination->getAllFileInfos())
+                files_to_remove.push_back(file_info.data_file_name);
+        }
+
+        writer->removeFilesAfterFailure(files_to_remove);
     }
-    else
+    catch (...)
     {
-        files_to_remove.push_back(".backup");
+        DB::tryLogCurrentException(__PRETTY_FUNCTION__);
-        for (const auto & file_info : coordination->getAllFileInfos())
-            files_to_remove.push_back(file_info.data_file_name);
     }
-
-    writer->removeFilesAfterFailure(files_to_remove);
 }
 
 }

src/Backups/BackupsWorker.cpp

@@ -100,10 +100,10 @@ UUID BackupsWorker::startMakingBackup(const ASTPtr & query, const ContextPtr & c
 
             /// Make a backup coordination.
             std::shared_ptr<IBackupCoordination> backup_coordination;
-            SCOPE_EXIT({
+            SCOPE_EXIT_SAFE(
                 if (backup_coordination && !backup_settings.internal)
                     backup_coordination->drop();
-            });
+            );
 
             ClusterPtr cluster;
             if (on_cluster)
@@ -278,10 +278,10 @@ UUID BackupsWorker::startRestoring(const ASTPtr & query, ContextMutablePtr conte
 
             /// Make a restore coordination.
             std::shared_ptr<IRestoreCoordination> restore_coordination;
-            SCOPE_EXIT({
+            SCOPE_EXIT_SAFE(
                 if (restore_coordination && !restore_settings.internal)
                     restore_coordination->drop();
-            });
+            );
 
             if (on_cluster && restore_settings.coordination_zk_path.empty())
             {