-
Notifications
You must be signed in to change notification settings - Fork 35
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
fix memory leak in SIOReader #99
Conversation
- delete previous Event and RunHeader before reading a new one - this restores the old expected behavior of LCIO - for parallel event reading one needs to use the MT::LCReader directly and handle the memory accordingly (e.g. via use of std::unique_ptr)
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.
That fixes the memory leak, but now it's not thread safe. Not sure if it was before, I haven't had a chance to test, yet.
The SIOReader implementing the IO:LCReader interface cannot be thread-safe and handle the memory for the user at the same time. |
Apparently exactly this commit breaks the LCIO Interface of WHIZARD. Trying to delete the _currentEvent leads to a crash. Not sure whether the commit is wrong, or this is an error in our interface since the beginning. |
Hmm, I don't observe a crash, but the commit surely doesn't guard against double deletes. The |
Actually, it crashes whenever we try to read the first event, not the last one. You are right, the if-clause around the delete is unnecessary. I'm note sure it really is a double delete. Maybe the crash is because this pointer comes out of an external code. The error message is:
|
That message is a bit weird. Line 108, the one with the error is the return. The delete happens in line 104. Is this code identical to tag 02-15-03? |
I just inserted some debug lines, in the original LCIO 02-15-03 it is line 104. BTW, I opened a new issue. |
If I keep the delete statement, our code also works if I replace: |
Not sure why that would work. You are just shadowing the member variable with a local variable with the same name. I can't say I understand exactly what's going on, but reading this: https://en.cppreference.com/w/cpp/memory/unique_ptr, I get
If that's the case, then the delete destroys the object, and then the assignment calls the destructor again? |
BEGINRELEASENOTES
IO::LCReader
MT::LCReader
directly andhandle the memory accordingly (e.g. via use of std::unique_ptr)
- see $LCIO/src/cpp/src/EXAMPLE/lcio_parallel_processing.cc
ENDRELEASENOTES