From d1fd590be17370f29a6545db32a13c0a9b14e9b1 Mon Sep 17 00:00:00 2001 From: leventov Date: Tue, 27 Dec 2016 15:38:52 +0700 Subject: [PATCH] In methods which return contexts, wrap all code after context is obtained in try-catch and try to close the context if any exception occurred, to prevent context leaks --- .../hash/impl/stage/hash/Chaining.java | 28 ++- .../chronicle/map/VanillaChronicleMap.java | 207 ++++++++++++------ 2 files changed, 159 insertions(+), 76 deletions(-) diff --git a/src/main/java/net/openhft/chronicle/hash/impl/stage/hash/Chaining.java b/src/main/java/net/openhft/chronicle/hash/impl/stage/hash/Chaining.java index 7780de150..95c081ee9 100644 --- a/src/main/java/net/openhft/chronicle/hash/impl/stage/hash/Chaining.java +++ b/src/main/java/net/openhft/chronicle/hash/impl/stage/hash/Chaining.java @@ -98,8 +98,8 @@ public abstract class Chaining extends ChainingInterface { @Override public void initUsed(boolean used, VanillaChronicleMap map) { assert used; - initMap(map); firstContextLockedInThisThread = rootContextInThisThread.lockContextLocally(map); + initMap(map); this.used = true; } @@ -112,13 +112,12 @@ public abstract class Chaining extends ChainingInterface { @Override public T getContext( - Class contextClass, BiFunction createChaining, + Class contextClass, BiFunction createChaining, VanillaChronicleMap map) { for (ChainingInterface context : contextChain) { if (context.getClass() == contextClass && !context.usedInit()) { - context.initUsed(true, map); - //noinspection unchecked - return (T) context; + return initUsedAndReturn(map, context); } } int maxNestedContexts = 1 << 10; @@ -132,7 +131,22 @@ public abstract class Chaining extends ChainingInterface { } //noinspection unchecked T context = createChaining.apply(this, map); - context.initUsed(true, map); - return context; + return initUsedAndReturn(map, context); + } + + private static T initUsedAndReturn( + VanillaChronicleMap map, ChainingInterface context) { + try { + context.initUsed(true, map); + //noinspection unchecked + return (T) context; + } catch (Throwable throwable) { + try { + ((AutoCloseable) context).close(); + } catch (Throwable t) { + throwable.addSuppressed(t); + } + throw throwable; + } } } diff --git a/src/main/java/net/openhft/chronicle/map/VanillaChronicleMap.java b/src/main/java/net/openhft/chronicle/map/VanillaChronicleMap.java index 3515df994..1ddbfe430 100755 --- a/src/main/java/net/openhft/chronicle/map/VanillaChronicleMap.java +++ b/src/main/java/net/openhft/chronicle/map/VanillaChronicleMap.java @@ -21,6 +21,7 @@ import net.openhft.chronicle.algo.hashing.LongHashFunction; import net.openhft.chronicle.bytes.Bytes; import net.openhft.chronicle.bytes.BytesStore; import net.openhft.chronicle.bytes.PointerBytesStore; +import net.openhft.chronicle.core.Jvm; import net.openhft.chronicle.core.io.Closeable; import net.openhft.chronicle.hash.ChronicleHashClosedException; import net.openhft.chronicle.hash.Data; @@ -28,6 +29,7 @@ import net.openhft.chronicle.hash.impl.*; import net.openhft.chronicle.hash.impl.stage.entry.LocksInterface; import net.openhft.chronicle.hash.impl.stage.hash.ChainingInterface; import net.openhft.chronicle.hash.impl.util.Objects; +import net.openhft.chronicle.hash.impl.util.Throwables; import net.openhft.chronicle.hash.serialization.DataAccess; import net.openhft.chronicle.hash.serialization.SizeMarshaller; import net.openhft.chronicle.hash.serialization.SizedReader; @@ -312,10 +314,19 @@ public class VanillaChronicleMap ContextHolder contextHolder = cxt.get(); if (contextHolder == null) { ChainingInterface queryContext = newQueryContext(); - contextHolder = new ContextHolder(queryContext); - addContext(contextHolder); - cxt.set(contextHolder); - return queryContext; + try { + contextHolder = new ContextHolder(queryContext); + addContext(contextHolder); + cxt.set(contextHolder); + return queryContext; + } catch (Throwable throwable) { + try { + ((AutoCloseable) queryContext).close(); + } catch (Throwable t) { + throwable.addSuppressed(t); + } + throw throwable; + } } ChainingInterface queryContext = contextHolder.get(); if (queryContext != null) { @@ -345,10 +356,19 @@ public class VanillaChronicleMap ContextHolder contextHolder = cxt.get(); if (contextHolder == null) { ChainingInterface iterationContext = newIterationContext(); - contextHolder = new ContextHolder(iterationContext); - addContext(contextHolder); - cxt.set(contextHolder); - return iterationContext; + try { + contextHolder = new ContextHolder(iterationContext); + addContext(contextHolder); + cxt.set(contextHolder); + return iterationContext; + } catch (Throwable throwable) { + try { + ((AutoCloseable) iterationContext).close(); + } catch (Throwable t) { + throwable.addSuppressed(t); + } + throw throwable; + } } ChainingInterface iterationContext = contextHolder.get(); if (iterationContext != null) { @@ -376,17 +396,35 @@ public class VanillaChronicleMap @NotNull public QueryContextInterface queryContext(Object key) { checkKey(key); - QueryContextInterface q = mapContext(); - q.initInputKey(q.inputKeyDataAccess().getData((K) key)); - return q; + QueryContextInterface c = mapContext(); + try { + c.initInputKey(c.inputKeyDataAccess().getData((K) key)); + return c; + } catch (Throwable throwable) { + try { + c.close(); + } catch (Throwable t) { + throwable.addSuppressed(t); + } + throw throwable; + } } @Override @NotNull public QueryContextInterface queryContext(Data key) { - QueryContextInterface q = mapContext(); - q.initInputKey(key); - return q; + QueryContextInterface c = mapContext(); + try { + c.initInputKey(key); + return c; + } catch (Throwable throwable) { + try { + c.close(); + } catch (Throwable t) { + throwable.addSuppressed(t); + } + throw throwable; + } } @NotNull @@ -394,16 +432,34 @@ public class VanillaChronicleMap public ExternalMapQueryContext queryContext( BytesStore keyBytes, long offset, long size) { Objects.requireNonNull(keyBytes); - QueryContextInterface q = mapContext(); - q.initInputKey(q.getInputKeyBytesAsData(keyBytes, offset, size)); - return q; + QueryContextInterface c = mapContext(); + try { + c.initInputKey(c.getInputKeyBytesAsData(keyBytes, offset, size)); + return c; + } catch (Throwable throwable) { + try { + c.close(); + } catch (Throwable t) { + throwable.addSuppressed(t); + } + throw throwable; + } } @Override public MapSegmentContext segmentContext(int segmentIndex) { IterationContext c = iterationContext(); - c.initSegmentIndex(segmentIndex); - return c; + try { + c.initSegmentIndex(segmentIndex); + return c; + } catch (Throwable throwable) { + try { + c.close(); + } catch (Throwable t) { + throwable.addSuppressed(t); + } + throw throwable; + } } @Override @@ -420,71 +476,84 @@ public class VanillaChronicleMap private V optimizedGet(Object key, V using) { checkKey(key); - CompiledMapQueryContext q = (CompiledMapQueryContext) mapContext(); - Data inputKey = q.inputKeyDataAccess().getData((K) key); + CompiledMapQueryContext c = (CompiledMapQueryContext) mapContext(); + boolean needReadUnlock = false; + Throwable primaryExc = null; + long segmentHeaderAddress = 0; try { - Throwable primaryExc = null; + Data inputKey = c.inputKeyDataAccess().getData((K) key); long inputKeySize = inputKey.size(); long keyHash = inputKey.hash(LongHashFunction.xx_r39()); HashSplitting hs = this.hashSplitting; int segmentIndex = hs.segmentIndex(keyHash); - long segmentHeaderAddress = segmentHeaderAddress(segmentIndex); - boolean needReadUnlock = false; - try { - CompactOffHeapLinearHashTable hl = this.hashLookup; - long searchKey = hl.maskUnsetKey(hs.segmentHash(keyHash)); - long searchStartPos = hl.hlPos(searchKey); - boolean needReadLock = true; - initLocks: - { - int indexOfThisContext = q.indexInContextChain; - for (int i = 0, size = q.contextChain.size(); i < size; i++) { - if (i == indexOfThisContext) - continue; - LocksInterface c = ((LocksInterface) (q.contextChain.get(i))); - if (c.segmentHeaderInit() && - c.segmentHeaderAddress() == segmentHeaderAddress && - c.locksInit()) { - LocksInterface root = c.rootContextLockedOnThisSegment(); - if (root.totalReadLockCount() > 0 || root.totalUpdateLockCount() > 0 || - root.totalWriteLockCount() > 0) { - needReadLock = false; - break initLocks; - } + segmentHeaderAddress = segmentHeaderAddress(segmentIndex); + CompactOffHeapLinearHashTable hl = this.hashLookup; + long searchKey = hl.maskUnsetKey(hs.segmentHash(keyHash)); + long searchStartPos = hl.hlPos(searchKey); + boolean needReadLock = true; + initLocks: + { + int indexOfThisContext = c.indexInContextChain; + for (int i = 0, size = c.contextChain.size(); i < size; i++) { + if (i == indexOfThisContext) + continue; + LocksInterface locks = ((LocksInterface) (c.contextChain.get(i))); + if (locks.segmentHeaderInit() && + locks.segmentHeaderAddress() == segmentHeaderAddress && + locks.locksInit()) { + LocksInterface root = locks.rootContextLockedOnThisSegment(); + if (root.totalReadLockCount() > 0 || root.totalUpdateLockCount() > 0 || + root.totalWriteLockCount() > 0) { + needReadLock = false; + break initLocks; } } } - if (needReadLock) { - BigSegmentHeader.INSTANCE.readLock(segmentHeaderAddress); - needReadUnlock = true; - } - return tieredValue(q, segmentHeaderAddress, segmentIndex, searchKey, searchStartPos, - inputKeySize, inputKey, using); - } catch (Throwable t) { - primaryExc = t; - throw t; - } finally { - if (primaryExc != null) { - try { - getClose(q, segmentHeaderAddress, needReadUnlock); - } catch (Throwable suppressedExc) { - primaryExc.addSuppressed(suppressedExc); - } - } else { - getClose(q, segmentHeaderAddress, needReadUnlock); - } } + if (needReadLock) { + BigSegmentHeader.INSTANCE.readLock(segmentHeaderAddress); + needReadUnlock = true; + } + return tieredValue(c, segmentHeaderAddress, segmentIndex, searchKey, searchStartPos, + inputKeySize, inputKey, using); + } catch (Throwable t) { + primaryExc = t; + throw t; } finally { - q.doCloseInputKeyDataAccess(); + if (primaryExc != null) { + try { + getClose(c, segmentHeaderAddress, needReadUnlock); + } catch (Throwable suppressedExc) { + primaryExc.addSuppressed(suppressedExc); + } + } else { + getClose(c, segmentHeaderAddress, needReadUnlock); + } } } - private void getClose(CompiledMapQueryContext q, long segmentHeaderAddress, + private void getClose(CompiledMapQueryContext c, long segmentHeaderAddress, boolean needReadUnlock) { - if (needReadUnlock) - BigSegmentHeader.INSTANCE.readUnlock(segmentHeaderAddress); - q.doCloseUsed(); + Throwable thrown = null; + try { + if (needReadUnlock) + BigSegmentHeader.INSTANCE.readUnlock(segmentHeaderAddress); + } catch (Throwable t) { + thrown = t; + } + try { + c.doCloseUsed(); + } catch (Throwable t) { + thrown = Throwables.returnOrSuppress(thrown, t); + } + try { + c.doCloseInputValueDataAccess(); + } catch (Throwable t) { + thrown = Throwables.returnOrSuppress(thrown, t); + } + if (thrown != null) + throw Jvm.rethrow(thrown); } private V tieredValue(CompiledMapQueryContext q,