CNDB-11092 main: Fix IndexInputLeakDetector #1310
Conversation
| private static final IndexFileUtils instance = new IndexFileUtils(defaultWriterOption); | ||
| private static IndexFileUtils overrideInstance = null; |
There was a problem hiding this comment.
It's a bit nitpicking, but is there any particular reason we use a combination of two variables instead of just marking the first one non-final and updating it in place? Then we wouldn't need to change all the stuff from .instance to .instance() (which is fine, but I'm worried it increases the conflict area in any merges we do e.g. wit apache). Is there any downside of using a single field?
There was a problem hiding this comment.
Not sure why it was done this way, this comes from a previous set of changes in main-5.0. Agree on the single variable. Regarding having getters and setters, the only reason that occurs to me is perhaps having less visibility in the setter? In any case, I have changed it to the simplest approach.
By the way, regarding Apache we have CASSANDRA-19961 to update this there too, albeit in that case the problem is that some inputs are not being closed.
|
|
||
| public class TrackingIndexFileUtils extends IndexFileUtils | ||
| { | ||
| private final Map<TrackingIndexInput, String> openInputs = Collections.synchronizedMap(new HashMap<>()); | ||
| private final Set<TrackingIndexInput> closedInputs = new HashSet<>(); |
There was a problem hiding this comment.
bug: That should be synchronized set, as inputs can be closed by any threads. The synchronized on close does not cut it because it does not synchronize on this common utils object, but it synchronizes on the individual IndexInput. So two different index inputs closing at the same time would attempt to insert into this set concurrently.
|
Mmh, it seems the fixed detector has found some leaks. Looking into it. |
|
The found leak was due to |
|
It seems the issues reported by Sonar are due to the changes around |
|
Fix IndexInputLeakDetector, which has been mostly no-op for a long time. It detects opened index inputs that haven't been closed. It seemed originally designed to also detect attempts at closing index inputs that had already been closed, but the current usage seems to do that kind of duplicated closing, which is probably ok if the operation is idempotent.