Fix some resource leak in tests #12794
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #12794 +/- ##
============================================
+ Coverage 61.75% 61.98% +0.23%
+ Complexity 207 198 -9
============================================
Files 2436 2461 +25
Lines 133233 134772 +1539
Branches 20636 20822 +186
============================================
+ Hits 82274 83542 +1268
- Misses 44911 45063 +152
- Partials 6048 6167 +119
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -480,6 +480,8 @@ public void testBigDecimalTooBig() { | |||
| Assert.assertThrows(IllegalArgumentException.class, () -> { | |||
| mutableSegmentImpl.index(row, defaultMetadata); | |||
| }); | |||
|
|
|||
| mutableSegmentImpl.destroy(); | |||
There was a problem hiding this comment.
This won't destroy the segment if the test fails
There was a problem hiding this comment.
Currently we don't take care of resource leak during test failures. That can be addressed separately
| _closed = true; | ||
| } | ||
| } | ||
| Unsafer.UNSAFE.freeMemory(_address); |
There was a problem hiding this comment.
I think we have to recover the synchronization here. Otherwise a race condition may try to free an address that was already cleaned. That could end up producing a SIGSEV which would kill the server.
We can improve the code using double check
There was a problem hiding this comment.
The whole method is changed to synchronized. I don't remember where I read it, but for modern JVM double checking synchronization is not necessary, especially when we almost always only call it once.
There was a problem hiding this comment.
You are right, I didn't see that. Anyway, in the other PR I've changed that so we don't need to synchronize in case one thread close this after another.
What newer jvms does for a fact is to make synchronized by default is specially expensive. This is due to:
- Virtual threads being pinned with synchronize. This is secondary given we don't use them and it should be something that eventually will be improved in (even) more modern JVMs
- +UseBiasedLocking has been disabled since Java 15 and the plan is to remove it. See https://openjdk.org/jeps/374, this and this
There was a problem hiding this comment.
Why would multiple threads trying to close the same buffer? If that happens, that means the buffer ownership is messed up. In regular case, this method should only be invoked exactly once.
I think double-checked locking might have performance gain when used in a singleton, but not here
There was a problem hiding this comment.
Ok, lets close the other PR then.
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/memory/unsafe/MmapMemory.java
Show resolved
Hide resolved
There was a problem hiding this comment.
I've created #12808 to apply the changes I proposed in the comments I've added here.
BTW, Foreign Memory API has been marked as production ready in recently released Java 22. It would be great to have a third alternative where that API is used (only if runtime is >= 22) and probably we should start thinking on removing LArray.
Under topic #12793
DirectMemory