[rfile] Throw exception in the factory methods when file cannot be opened

Author: silverweed

io/io/inc/ROOT/RFile.hxx

@@ -131,13 +131,18 @@ public:
 
    ///// Factory methods /////
 
-   /// Opens the file for reading
+   /// Opens the file for reading. `path` may be a regular file path or a remote URL.
+   /// \throw ROOT::RException if the file at `path` could not be opened.
    static std::unique_ptr<RFile> Open(std::string_view path);
 
-   /// Opens the file for reading/writing, overwriting it if it already exists
+   /// Opens the file for reading/writing, overwriting it if it already exists.
+   /// \throw ROOT::RException if a file could not be created at `path` (e.g. if the specified
+   /// directory tree does not exist).
    static std::unique_ptr<RFile> Recreate(std::string_view path);
 
-   /// Opens the file for updating
+   /// Opens the file for updating, creating a new one if it doesn't exist.
+   /// \throw ROOT::RException if the file at `path` could neither be read nor created
+   /// (e.g. if the specified directory tree does not exist).
    static std::unique_ptr<RFile> Update(std::string_view path);
 
    ///// Instance methods /////
@@ -185,7 +190,7 @@ public:
       PutInternal(path, obj, flags);
    }
 
-   /// Writes all objects to disk with the file structure.
+   /// Writes all objects and the file structure to disk.
    /// Returns the number of bytes written.
    size_t Flush();
 

io/io/src/RFile.cxx

@@ -190,6 +190,9 @@ std::unique_ptr<RFile> RFile::Open(std::string_view path)
 
    TDirectory::TContext ctx(nullptr); // XXX: probably not thread safe?
    auto tfile = std::unique_ptr<TFile>(TFile::Open(std::string(path).c_str(), "READ_WITHOUT_GLOBALREGISTRATION"));
+   if (!tfile || tfile->IsZombie())
+      throw ROOT::RException(R__FAIL("failed to open file " + std::string(path) + " for reading"));
+
    auto rfile = std::unique_ptr<RFile>(new RFile(std::move(tfile)));
    return rfile;
 }
@@ -200,6 +203,9 @@ std::unique_ptr<RFile> RFile::Update(std::string_view path)
 
    TDirectory::TContext ctx(nullptr); // XXX: probably not thread safe?
    auto tfile = std::unique_ptr<TFile>(TFile::Open(std::string(path).c_str(), "UPDATE_WITHOUT_GLOBALREGISTRATION"));
+   if (!tfile || tfile->IsZombie())
+      throw ROOT::RException(R__FAIL("failed to open file " + std::string(path) + " for updating"));
+
    auto rfile = std::unique_ptr<RFile>(new RFile(std::move(tfile)));
    return rfile;
 }
@@ -210,6 +216,9 @@ std::unique_ptr<RFile> RFile::Recreate(std::string_view path)
 
    TDirectory::TContext ctx(nullptr); // XXX: probably not thread safe?
    auto tfile = std::unique_ptr<TFile>(TFile::Open(std::string(path).c_str(), "RECREATE_WITHOUT_GLOBALREGISTRATION"));
+   if (!tfile || tfile->IsZombie())
+      throw ROOT::RException(R__FAIL("failed to open file " + std::string(path) + " for writing"));
+
    auto rfile = std::unique_ptr<RFile>(new RFile(std::move(tfile)));
    return rfile;
 }
@@ -281,7 +290,7 @@ void *RFile::GetUntyped(std::string_view pathSV, const std::type_info &type) con
       if (auto autoAddFunc = cls->GetDirectoryAutoAdd(); autoAddFunc) {
          autoAddFunc(obj, nullptr);
       }
-   } else if (key && !GetROOT()->IsBatch()) { // XXX: do we really want this warning?
+   } else if (key && !GetROOT()->IsBatch()) {
       R__LOG_WARNING(RFileLog()) << "Tried to get object '" << path << "' of type " << cls->GetName()
                                  << " but that path contains an object of type " << key->GetClassName();
    }

io/io/test/rfile.cxx

@@ -4,6 +4,7 @@
 #include <TFile.h>
 #include <TH1D.h>
 #include <TROOT.h>
+#include <TSystem.h>
 #include <ROOT/RError.hxx>
 #include <ROOT/RFile.hxx>
 #include <ROOT/TestSupport.hxx>
@@ -69,6 +70,40 @@ TEST(RFile, Open)
    EXPECT_THROW(file->Put("foo", foo), ROOT::RException);
 }
 
+TEST(RFile, OpenInexistent)
+{
+   FileRaii fileGuard("does_not_exist.root");
+
+   // make sure that the file really does not exist, in case a previous test didn't clean it up.
+   gSystem->Unlink(fileGuard.GetPath().c_str());
+
+   ROOT::TestSupport::CheckDiagsRAII diags;
+   diags.optionalDiag(kSysError, "TFile::TFile", "", false);
+   diags.optionalDiag(kError, "TFile::TFile", "", false);
+   
+   try {
+      auto f = RFile::Open("does_not_exist.root");
+      FAIL() << "trying to open an inexistent file should throw";
+   } catch (const ROOT::RException &e) {
+      EXPECT_THAT(e.what(), testing::HasSubstr("failed to open file"));
+   }
+   try {
+      auto f = RFile::Update("/a/random/directory/that/definitely/does_not_exist.root");
+      FAIL() << "trying to update a file under an inexistent directory should throw";
+   } catch (const ROOT::RException &e) {
+      EXPECT_THAT(e.what(), testing::HasSubstr("failed to open file"));
+   }
+   try {
+      auto f = RFile::Recreate("/a/random/directory/that/definitely/does_not_exist.root");
+      FAIL() << "trying to create a file under an inexistent directory should throw";
+   } catch (const ROOT::RException &e) {
+      EXPECT_THAT(e.what(), testing::HasSubstr("failed to open file"));
+   }
+
+   // This succeeds because Update creates the file if it doesn't exist.
+   EXPECT_NO_THROW(RFile::Update("does_not_exist.root"));
+}
+
 TEST(RFile, OpenForWriting)
 {
    FileRaii fileGuard("test_rfile_write.root");