diff --git a/io/io/src/TFileCacheRead.cxx b/io/io/src/TFileCacheRead.cxx index ec859aa0d72e6..65deceaeabf65 100644 --- a/io/io/src/TFileCacheRead.cxx +++ b/io/io/src/TFileCacheRead.cxx @@ -460,6 +460,8 @@ Int_t TFileCacheRead::ReadBufferExtNormal(char *buf, Long64_t pos, Int_t len, In if (!fAsyncReading) { // Then we use the vectored read to read everything now if (fFile->ReadBuffers(fBuffer,fPos,fLen,fNb)) { + // fBuffer does not hold the blocks: forget them, so that they are read from the file instead + TFileCacheRead::Prefetch(0, 0); return -1; } fIsTransferred = kTRUE; diff --git a/tree/tree/src/TTreeCacheUnzip.cxx b/tree/tree/src/TTreeCacheUnzip.cxx index d3783c58d86dd..0d621ea49fe42 100644 --- a/tree/tree/src/TTreeCacheUnzip.cxx +++ b/tree/tree/src/TTreeCacheUnzip.cxx @@ -787,7 +787,8 @@ Int_t TTreeCacheUnzip::GetUnzipBuffer(char **buf, Long64_t pos, Int_t len, bool } res = 0; - if (!ReadBufferExt(fCompBuffer, pos, len, loc)) { + // Read the buffer from the file unless it was found in the cache, including when reading the cache failed + if (ReadBufferExt(fCompBuffer, pos, len, loc) != 1) { // Cache is invalidated and we need to wait for all unzipping tasks to be finished before fill new baskets in cache. #ifdef R__USE_IMT if(ROOT::IsImplicitMTEnabled() && fUnzipTaskGroup) { diff --git a/tree/tree/test/CMakeLists.txt b/tree/tree/test/CMakeLists.txt index 4e903b06daafc..94ece5abc821c 100644 --- a/tree/tree/test/CMakeLists.txt +++ b/tree/tree/test/CMakeLists.txt @@ -38,6 +38,7 @@ ROOT_ADD_GTEST(entrylist_enter entrylist_enter.cxx LIBRARIES RIO Tree) ROOT_ADD_GTEST(entrylist_enterrange entrylist_enterrange.cxx LIBRARIES RIO Tree) ROOT_ADD_GTEST(friendinfo friendinfo.cxx LIBRARIES RIO Tree) ROOT_ADD_GTEST(testTTreeCompression TTreeCompression.cxx LIBRARIES RIO Tree MathCore) +ROOT_ADD_GTEST(testTTreeCacheReadError TTreeCacheReadError.cxx LIBRARIES RIO Tree) ROOT_ADD_GTEST(evolution evolution.cxx LIBRARIES RIO Tree) ROOT_GENERATE_DICTIONARY(EvolutionStruct ${CMAKE_CURRENT_SOURCE_DIR}/EvolutionStruct.hxx MODULE evolution diff --git a/tree/tree/test/TTreeCacheReadError.cxx b/tree/tree/test/TTreeCacheReadError.cxx new file mode 100644 index 0000000000000..fc872ec23b84a --- /dev/null +++ b/tree/tree/test/TTreeCacheReadError.cxx @@ -0,0 +1,137 @@ +#include "ROOT/TestSupport.hxx" + +#include "TFile.h" +#include "TSystem.h" +#include "TTree.h" +#include "TTreeCacheUnzip.h" + +#include "gtest/gtest.h" + +#include + +namespace { + +/// A file whose `failAt`-th vectored read fails without filling the buffer, as a TNetXNGFile does on a socket timeout. +class FailingReadBuffersFile : public TFile { + int fCalls = 0; + int fFailAt; + +public: + FailingReadBuffersFile(const char *name, int failAt) : TFile(name), fFailAt(failAt) {} + + Bool_t ReadBuffers(char *buf, Long64_t *pos, Int_t *len, Int_t nbuf) override + { + if (++fCalls == fFailAt) + return kTRUE; + return TFile::ReadBuffers(buf, pos, len, nbuf); + } + + int GetCalls() const { return fCalls; } +}; + +class TTreeCacheReadError : public ::testing::Test { +protected: + static constexpr const char *kFileName = "ttreecache_readerror.root"; + static constexpr int kEntries = 20000; + // The size of a cache holding the baskets of a single cluster. + static constexpr int kCacheSize = 20000; + + static void SetUpTestSuite() + { + TFile f(kFileName, "RECREATE"); + TTree t("t", "t"); + int a; + std::vector v; + t.Branch("a", &a, 4000); + t.Branch("v", &v, 4000); + t.SetAutoFlush(2000); + for (int i = 0; i < kEntries; ++i) { + a = i; + v.assign(i % 7, i); + t.Fill(); + } + t.Write(); + } + + static void TearDownTestSuite() { gSystem->Unlink(kFileName); } + + /// Read all entries of `t`, checking their content, and return the number of entries that could not be read. + static int ReadAll(TTree &t) + { + int a = -1; + std::vector *v = nullptr; + t.SetBranchAddress("a", &a); + t.SetBranchAddress("v", &v); + int nErrors = 0; + for (int i = 0; i < kEntries; ++i) { + if (t.GetEntry(i) <= 0) { + ++nErrors; + continue; + } + EXPECT_EQ(a, i); + EXPECT_NE(v, nullptr); + if (!v) + continue; + EXPECT_EQ(v->size(), static_cast(i % 7)); + for (int x : *v) + EXPECT_EQ(x, i); + } + return nErrors; + } +}; + +/// Use a TTreeCacheUnzip for the tree while in scope. +class UnzipCacheRAII { +public: + UnzipCacheRAII(TTree &t, Long64_t cacheSize) + { + TTreeCacheUnzip::SetParallelUnzip(TTreeCacheUnzip::kEnable); + new TTreeCacheUnzip(&t, cacheSize); + } + ~UnzipCacheRAII() { TTreeCacheUnzip::SetParallelUnzip(TTreeCacheUnzip::kDisable); } +}; + +} // namespace + +// A failed read of the baskets of a cluster must not leave the cache serving the other baskets of that cluster from a +// buffer that was never filled. +TEST_F(TTreeCacheReadError, FailedReadBuffers) +{ + // The first vectored read is that of the cache in learning phase, the third one that of the second cluster. + FailingReadBuffersFile f(kFileName, /*failAt=*/3); + auto t = f.Get("t"); + ASSERT_NE(t, nullptr); + t->SetCacheSize(kCacheSize); + + ROOT::TestSupport::CheckDiagsRAII diags; + diags.requiredDiag(kError, "TBranch::GetBasket", "badread=1", /*matchFullMessage=*/false); + // Only the entry whose read failed is lost; the other baskets of its cluster are read from the file. + EXPECT_EQ(ReadAll(*t), 1); + EXPECT_GT(f.GetCalls(), 3); +} + +// The same for TTreeCacheUnzip, where the read that fails is retried by TBasket. +TEST_F(TTreeCacheReadError, FailedReadBuffersUnzip) +{ + FailingReadBuffersFile f(kFileName, /*failAt=*/3); + auto t = f.Get("t"); + ASSERT_NE(t, nullptr); + UnzipCacheRAII unzip(*t, kCacheSize); + + EXPECT_EQ(ReadAll(*t), 0); + EXPECT_GT(f.GetCalls(), 3); +} + +// When prefilling the cache during learning, the first basket is requested from TTreeCacheUnzip before its blocks are +// read; a failure to read them must not unzip whatever its buffer for compressed data holds. +TEST_F(TTreeCacheReadError, FailedReadBuffersUnzipLearnPrefill) +{ + FailingReadBuffersFile f(kFileName, /*failAt=*/1); + auto t = f.Get("t"); + ASSERT_NE(t, nullptr); + UnzipCacheRAII unzip(*t, kCacheSize); + t->GetReadCache(&f)->SetLearnPrefill(TTreeCache::kAllBranches); + + EXPECT_EQ(ReadAll(*t), 0); + EXPECT_GT(f.GetCalls(), 1); +}