From deea0ea7c128260f4128bd83d87f9528fb839e88 Mon Sep 17 00:00:00 2001 From: Vincenzo Eduardo Padulano Date: Tue, 6 Aug 2024 09:25:32 +0200 Subject: [PATCH 1/5] [io] Silently fallback if full xroot path is not found on EOS Sometimes the `getxattr` call returns a valid string, but the string is incomplete. Namely, part of the full URL to the file on EOS is missing (usually the filename itself). Workaround by checking that the xurl string ends with the filename and in case it is not present, avoid calling TFile::Open. Co-authored-by: Jonas Hahnfeld --- io/io/src/TFile.cxx | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/io/io/src/TFile.cxx b/io/io/src/TFile.cxx index 86e93e6457eb7..fa8592935c973 100644 --- a/io/io/src/TFile.cxx +++ b/io/io/src/TFile.cxx @@ -4116,13 +4116,19 @@ TFile *TFile::Open(const char *url, Option_t *options, const char *ftitle, ssize_t len = getxattr(fileurl.GetFile(), "eos.url.xroot", nullptr, 0); if (len > 0) { std::string xurl(len, 0); - if (getxattr(fileurl.GetFile(), "eos.url.xroot", &xurl[0], len) == len) { - if ((f = TFile::Open(xurl.c_str(), options, ftitle, compress, netopt))) { - if (!f->IsZombie()) { - return f; - } else { - delete f; - f = nullptr; + std::string fileNameFromUrl{fileurl.GetFile()}; + if (getxattr(fileNameFromUrl.c_str(), "eos.url.xroot", &xurl[0], len) == len) { + // Sometimes the `getxattr` call may return an invalid URL due + // to the POSIX attribute not being yet completely filled by EOS. + if (auto baseName = fileNameFromUrl.substr(fileNameFromUrl.find_last_of("/") + 1); + std::equal(baseName.crbegin(), baseName.crend(), xurl.crbegin())) { + if ((f = TFile::Open(xurl.c_str(), options, ftitle, compress, netopt))) { + if (!f->IsZombie()) { + return f; + } else { + delete f; + f = nullptr; + } } } } From 8158b40bd09114e7a144f9979073f7191ba47c6a Mon Sep 17 00:00:00 2001 From: Vincenzo Eduardo Padulano Date: Tue, 28 Jan 2025 13:59:18 +0100 Subject: [PATCH 2/5] [df] Fix interaction between RSampleInfo and redirected EOS paths When the input files are paths to FUSE-mounted EOS files, during the event loop these paths will be redirected to the corresponding xroot EOS URL, in TFile::Open. This was causing a bad interaction with the sample metadata and the subsequent usage in the event loop, e.g. through DefinePerSample. Specifically, we fill a map with the metadata at construction time, which includes the input file paths (without redirection). These will then be irretrievable during the event loop since the map key will not correspond to the redirected path. Fix this by also adding the redirected map during the filling of the map in ChangeSpec. --- tree/dataframe/src/RLoopManager.cxx | 56 +++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/tree/dataframe/src/RLoopManager.cxx b/tree/dataframe/src/RLoopManager.cxx index f0fae786055c6..ba16065253714 100644 --- a/tree/dataframe/src/RLoopManager.cxx +++ b/tree/dataframe/src/RLoopManager.cxx @@ -42,6 +42,25 @@ #include "ROOT/RNTupleDS.hxx" #endif +#ifdef R__UNIX +// Functions needed to perform EOS XRootD redirection in ChangeSpec +#include +#include "TEnv.h" +#include "TSystem.h" +#ifndef R__FBSD +#include +#else +#include +#endif +#ifdef R__MACOSX +/* On macOS getxattr takes two extra arguments that should be set to 0 */ +#define getxattr(path, name, value, size) getxattr(path, name, value, size, 0u, 0) +#endif +#ifdef R__FBSD +#define getxattr(path, name, value, size) extattr_get_file(path, EXTATTR_NAMESPACE_USER, name, value, size) +#endif +#endif + #include #include #include @@ -403,6 +422,38 @@ RLoopManager::RLoopManager(ROOT::RDF::Experimental::RDatasetSpec &&spec) ChangeSpec(std::move(spec)); } +#ifdef R__UNIX +namespace { +std::optional GetRedirectedSampleId(std::string_view path, std::string_view datasetName) +{ + // Mimick the redirection done in TFile::Open to see if the path points to a FUSE-mounted EOS path. + // If so, we create a redirected sample ID with the full xroot URL. + TString expandedUrl(path.data()); + gSystem->ExpandPathName(expandedUrl); + if (gEnv->GetValue("TFile.CrossProtocolRedirects", 1) == 1) { + TUrl fileurl(expandedUrl, /* default is file */ kTRUE); + if (strcmp(fileurl.GetProtocol(), "file") == 0) { + ssize_t len = getxattr(fileurl.GetFile(), "eos.url.xroot", nullptr, 0); + if (len > 0) { + std::string xurl(len, 0); + std::string fileNameFromUrl{fileurl.GetFile()}; + if (getxattr(fileNameFromUrl.c_str(), "eos.url.xroot", &xurl[0], len) == len) { + // Sometimes the `getxattr` call may return an invalid URL due + // to the POSIX attribute not being yet completely filled by EOS. + if (auto baseName = fileNameFromUrl.substr(fileNameFromUrl.find_last_of("/") + 1); + std::equal(baseName.crbegin(), baseName.crend(), xurl.crbegin())) { + return xurl + '/' + datasetName.data(); + } + } + } + } + } + + return std::nullopt; +} +} // namespace +#endif + /** * @brief Changes the internal TTree held by the RLoopManager. * @@ -441,6 +492,11 @@ void RLoopManager::ChangeSpec(ROOT::RDF::Experimental::RDatasetSpec &&spec) // is exposed to users via RSampleInfo and DefinePerSample). const auto sampleId = files[i] + '/' + trees[i]; fSampleMap.insert({sampleId, &sample}); +#ifdef R__UNIX + // Also add redirected EOS xroot URL when available + if (auto redirectedSampleId = GetRedirectedSampleId(files[i], trees[i])) + fSampleMap.insert({redirectedSampleId.value(), &sample}); +#endif } } SetTree(std::move(chain)); From e3b9e68774fbb3841ad286213169816ed0a2ea52 Mon Sep 17 00:00:00 2001 From: Vincenzo Eduardo Padulano Date: Tue, 25 Feb 2025 11:29:33 +0100 Subject: [PATCH 3/5] [tree] Fix interaction between GetEntries and GetListOfFriends Fixes https://github.com/root-project/root/issues/17820 Full analysis of the issue follows: 1. TChain::GetEntries calls TChain::LoadTree(TTree::kMaxEntries - 1) 2. Internally, this will set fReadEntry == -2, due to how LoadTree works and the fact it's trying to read beyond the last entry in the chain 3. This has an effect here https://github.com/root-project/root/blob/f33985dca2d6bc505e83498f7b4561ddd969be17/tree/tree/src/TChain.cxx#L1675-L1679 where `t` is the current friend being traversed in the list of friends of the *chain*. We call `t->LoadTree(TTree::kMaxEntries - 1)`. 4. Inside the call to LoadTree on the friend, we reach this line https://github.com/root-project/root/blob/f33985dca2d6bc505e83498f7b4561ddd969be17/tree/tree/src/TChain.cxx#L1484 where the fTree data member of the friend is invalidated. 5. Back to the LoadTree of the main chain, these lines https://github.com/root-project/root/blob/f33985dca2d6bc505e83498f7b4561ddd969be17/tree/tree/src/TChain.cxx#L1681-L1684 would normally connect the fTree data member of the main chain to the current friend, so the list of friends is properly populated. But due to 4., `friend->GetTree()` will return nullptr so the connection won't happen. This commit changes the implementation of TChain::GetEntries, so that after computing the total number of entries the chain is brought back to an invalid but recoverable state, and if the previous read entry was at least zero, the cursor is moved to it. Co-authored-by: Philippe Canal --- tree/tree/src/TChain.cxx | 9 +- tree/treeplayer/test/CMakeLists.txt | 2 + .../test/ttreeindex_getlistoffriends.cxx | 114 ++++++++++++++++++ 3 files changed, 124 insertions(+), 1 deletion(-) create mode 100644 tree/treeplayer/test/ttreeindex_getlistoffriends.cxx diff --git a/tree/tree/src/TChain.cxx b/tree/tree/src/TChain.cxx index 2533490418e3e..2a1e7084154aa 100644 --- a/tree/tree/src/TChain.cxx +++ b/tree/tree/src/TChain.cxx @@ -985,7 +985,14 @@ Long64_t TChain::GetEntries() const return fProofChain->GetEntries(); } if (fEntries == TTree::kMaxEntries) { - const_cast(this)->LoadTree(TTree::kMaxEntries-1); + const auto readEntry = fReadEntry; + auto *thisChain = const_cast(this); + thisChain->LoadTree(TTree::kMaxEntries - 1); + thisChain->InvalidateCurrentTree(); + if (readEntry >= 0) + thisChain->LoadTree(readEntry); + else + thisChain->fReadEntry = readEntry; } return fEntries; } diff --git a/tree/treeplayer/test/CMakeLists.txt b/tree/treeplayer/test/CMakeLists.txt index 0f53d7644284b..4edf2fda38cfc 100644 --- a/tree/treeplayer/test/CMakeLists.txt +++ b/tree/treeplayer/test/CMakeLists.txt @@ -23,3 +23,5 @@ endif() ROOT_ADD_GTEST(ttreeindex_clone ttreeindex_clone.cxx LIBRARIES TreePlayer) ROOT_ADD_GTEST(ttreereader_friends ttreereader_friends.cxx LIBRARIES TreePlayer) + +ROOT_ADD_GTEST(ttreeindex_getlistoffriends ttreeindex_getlistoffriends.cxx LIBRARIES TreePlayer) diff --git a/tree/treeplayer/test/ttreeindex_getlistoffriends.cxx b/tree/treeplayer/test/ttreeindex_getlistoffriends.cxx new file mode 100644 index 0000000000000..4685c93409c61 --- /dev/null +++ b/tree/treeplayer/test/ttreeindex_getlistoffriends.cxx @@ -0,0 +1,114 @@ +#include "TBranch.h" +#include "TChain.h" +#include "TCollection.h" // TRangeDynCast +#include "TFile.h" +#include "TFriendElement.h" +#include "TObjArray.h" +#include "TTree.h" + +#include "gtest/gtest.h" + +#include +#include +#include + +void write_data(std::string_view treename, std::string_view filename) +{ + TFile f{filename.data(), "update"}; + TTree t{treename.data(), treename.data()}; + int runNumber{}; + int eventNumber{}; + float val{}; + t.Branch("runNumber", &runNumber, "runNumber/I"); + t.Branch("eventNumber", &eventNumber, "eventNumber/I"); + t.Branch("val", &val, "val/F"); + if (treename == "main") { + for (auto rn = 0; rn < 3; rn++) { + runNumber = rn; + for (auto en = 0; en < 5; en++) { + eventNumber = en; + val = en * rn; + t.Fill(); + } + } + } else { + for (auto rn = 0; rn < 3; rn++) { + runNumber = rn; + for (auto en = 4; en >= 0; en--) { + eventNumber = en; + val = en * rn; + t.Fill(); + } + } + } + + f.Write(); +} + +struct TTreeIndexGH_17820 : public ::testing::Test { + constexpr static auto fFileName{"ttreeindex_getlistoffriends_gh_17820.root"}; + constexpr static auto fMainName{"main"}; + constexpr static auto fFriendName{"friend"}; + + static void SetUpTestCase() + { + write_data(fMainName, fFileName); + write_data(fFriendName, fFileName); + } + + static void TearDownTestCase() { std::remove(fFileName); } +}; + +void expect_branch_names(const TObjArray *branches, const std::vector &branchNames) +{ + auto nBranchNames = branchNames.size(); + decltype(nBranchNames) nBranches{}; + for (const auto *br : TRangeDynCast(branches)) { + EXPECT_STREQ(br->GetName(), branchNames[nBranches].c_str()); + nBranches++; + } + EXPECT_EQ(nBranches, nBranchNames); +} + +// Regression test for https://github.com/root-project/root/issues/17820 +TEST_F(TTreeIndexGH_17820, RunTest) +{ + TChain mainChain{fMainName}; + mainChain.AddFile(fFileName); + + TChain friendChain{fFriendName}; + friendChain.AddFile(fFileName); + friendChain.BuildIndex("runNumber", "eventNumber"); + + mainChain.AddFriend(&friendChain); + + // Calling GetEntries used to mess with the fTree data member of the main + // chain, not connecting it to the friend chain and thus losing the list + // of friends. This in turn corrupted the list of branches. + mainChain.GetEntries(); + + const auto *listOfBranches = mainChain.GetListOfBranches(); + ASSERT_TRUE(listOfBranches); + + const std::vector expectedNames{"runNumber", "eventNumber", "val"}; + + expect_branch_names(listOfBranches, expectedNames); + + const auto *curTree = mainChain.GetTree(); + ASSERT_TRUE(curTree); + + const auto *listOfFriends = mainChain.GetTree()->GetListOfFriends(); + ASSERT_TRUE(listOfFriends); + EXPECT_EQ(listOfFriends->GetEntries(), 1); + + auto *friendTree = dynamic_cast(dynamic_cast(listOfFriends->At(0))->GetTree()); + ASSERT_TRUE(friendTree); + + EXPECT_STREQ(friendTree->GetName(), fFriendName); + const auto *friendFile = friendTree->GetCurrentFile(); + ASSERT_TRUE(friendFile); + EXPECT_STREQ(friendFile->GetName(), fFileName); + + const auto *friendBranches = friendTree->GetListOfBranches(); + expect_branch_names(friendBranches, expectedNames); +} From 66d44a63086cc75afcde01adb03e41a8a371e18e Mon Sep 17 00:00:00 2001 From: Philippe Canal Date: Mon, 3 Mar 2025 14:45:19 -0600 Subject: [PATCH 4/5] TChain::GetEntries apply FriendLockStatus. Avoid calling LoadTree when it will be a no-op and especially don't undo work we wont be able to redo --- tree/tree/src/TChain.cxx | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tree/tree/src/TChain.cxx b/tree/tree/src/TChain.cxx index 2a1e7084154aa..afa8282e18764 100644 --- a/tree/tree/src/TChain.cxx +++ b/tree/tree/src/TChain.cxx @@ -985,6 +985,10 @@ Long64_t TChain::GetEntries() const return fProofChain->GetEntries(); } if (fEntries == TTree::kMaxEntries) { + // If the following is true, we are within a recursion about friend, + // and `LoadTree` will be no-op. + if (kLoadTree & fFriendLockStatus) + return fEntries; const auto readEntry = fReadEntry; auto *thisChain = const_cast(this); thisChain->LoadTree(TTree::kMaxEntries - 1); From 8c05b8c397cc50960dfb75e979da75e3ed7479cf Mon Sep 17 00:00:00 2001 From: Philippe Canal Date: Mon, 3 Mar 2025 14:47:51 -0600 Subject: [PATCH 5/5] TTreeIndex::GetEntryNumberFriend don't call GetEntriesFast. Calling GetEntriesFast is enough to know if you reach past the end (if we are in the last file then it will return the precise number or something too large if we are far). Calling GetEntries can move the cursor of the chain and thus change the state of the parent. --- tree/treeplayer/src/TTreeIndex.cxx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tree/treeplayer/src/TTreeIndex.cxx b/tree/treeplayer/src/TTreeIndex.cxx index 15d6f248012e0..db0db91c6f73c 100644 --- a/tree/treeplayer/src/TTreeIndex.cxx +++ b/tree/treeplayer/src/TTreeIndex.cxx @@ -345,7 +345,7 @@ Long64_t TTreeIndex::GetEntryNumberFriend(const TTree *parent) if (!parent) return -3; // We reached the end of the parent tree Long64_t pentry = parent->GetReadEntry(); - if (pentry >= parent->GetEntries()) + if (pentry >= parent->GetEntriesFast()) return -2; GetMajorFormulaParent(parent); GetMinorFormulaParent(parent);