-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[tree] Fix interaction between GetEntries and GetListOfFriends #17822
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[tree] Fix interaction between GetEntries and GetListOfFriends #17822
Conversation
Note that the current change apparently fixed the attached reproducer on my local machine, but evidently something is still missing. Suggestions on how to proceed are welcome! |
Test Results 18 files 18 suites 4d 3h 32m 23s ⏱️ Results for commit 7af23c6. ♻️ This comment has been updated with latest results. |
57f1841
to
6c75a77
Compare
4095d0c
to
5fbb4b1
Compare
|
Fixes root-project#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 <[email protected]>
5fbb4b1
to
dd06918
Compare
Avoid calling LoadTree when it will be a no-op and especially don't undo work we wont be able to redo
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.
Thanks. I rebuilt locally ROOT in Debug mode using master + this patch but I still see the same crash. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Fixes #17820
Full analysis of the issue follows:
TChain::GetEntries calls TChain::LoadTree(TTree::kMaxEntries - 1)
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
This has an effect here
root/tree/tree/src/TChain.cxx
Lines 1675 to 1679 in f33985d
t
is the current friend being traversed in the list of friends of the chain. We callt->LoadTree(TTree::kMaxEntries - 1)
.Inside the call to LoadTree on the friend, we reach this line
root/tree/tree/src/TChain.cxx
Line 1484 in f33985d
Back to the LoadTree of the main chain, these lines
root/tree/tree/src/TChain.cxx
Lines 1681 to 1684 in f33985d
friend->GetTree()
will return nullptr so the connection won't happen.