Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fixing Geoloader leak probably introduced new bugs #1514

Open
ChristianTackeGSI opened this issue Apr 2, 2024 · 4 comments
Open

Fixing Geoloader leak probably introduced new bugs #1514

ChristianTackeGSI opened this issue Apr 2, 2024 · 4 comments
Milestone
next

Comments

@ChristianTackeGSI
Copy link
Member

It looks like fixing the geoloader leak in #1444 introduced a new issue: https://cdash.gsi.de/testDetails.php?test=19713169&build=448354

I was not quickly able to track down what exactly is the problem here. But it seems to be related to FairGeoNodes probably being deleted and then used.
@dennisklein dennisklein added this to the v19.0 milestone Apr 3, 2024
@ChristianTackeGSI ChristianTackeGSI mentioned this issue Apr 3, 2024
Draft
1 task
@dennisklein
Copy link
Member

https://cdash.gsi.de/testDetails.php?test=19728299&build=448681 This shows also a segfault which is unrelated to your geoloader patches (v18.6_patches).

@dennisklein
Copy link
Member

nvm, i confused the PR numbers...

@ChristianTackeGSI
Copy link
Member Author

ChristianTackeGSI commented Apr 4, 2024 •
edited

BTW, if we can't fix this for the 19.0 we should back out the geoloader stuff by

/// \bug Leaks GeoLoader and related resources, prevents memory issues (probably a double free)
///      See: https://github.com/FairRootGroup/FairRoot/issues/1514
(void)fGeoLoader.release();

in the FairRunSim dtor.

ChristianTackeGSI added a commit to ChristianTackeGSI/FairRoot that referenced this issue Apr 5, 2024
@ChristianTackeGSI
workaround: Avoid geoloader memory issues
3b3d868 
Not leaking fGeoLoader on FairRunSim seems to cause
some memory issues (probbaly a double free/delete).
Until we find it, let's leak the Geoloader, as before.

See: FairRootGroup#1514
See: ede7137
@ChristianTackeGSI ChristianTackeGSI mentioned this issue Apr 5, 2024
Merged
1 task
ChristianTackeGSI added a commit to ChristianTackeGSI/FairRoot that referenced this issue Apr 16, 2024
@ChristianTackeGSI
workaround: Avoid geoloader memory issues
2766d63 
Not leaking fGeoLoader on FairRunSim seems to cause
some memory issues (probbaly a double free/delete).
Until we find it, let's leak the Geoloader, as before.

See: FairRootGroup#1514
See: ede7137
dennisklein pushed a commit that referenced this issue Apr 16, 2024
@ChristianTackeGSI @dennisklein
workaround: Avoid geoloader memory issues
e58c820 
Not leaking fGeoLoader on FairRunSim seems to cause
some memory issues (probbaly a double free/delete).
Until we find it, let's leak the Geoloader, as before.

See: #1514
See: ede7137
@ChristianTackeGSI
Copy link
Member Author

For the time being #1518 has re-enabled the memory (including a proper marking). Still the problem of the unproper memory handling needs to be fixed.

ChristianTackeGSI added a commit to ChristianTackeGSI/FairRoot that referenced this issue Apr 21, 2024
@ChristianTackeGSI
workaround: Leak GeoSets in GeoInterface
2668ed5 
Instead of leaking the complete FairGeoLoader, only leak
the FairGeoSets in FairGeoInterface.

Still does not fix FairRootGroup#1514

See: FairRootGroup#1514
@dennisklein dennisklein modified the milestones: v19.0, v19.2 May 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
next
Development

No branches or pull requests
2 participants
@dennisklein @ChristianTackeGSI