From b4258fa263f76d32323d74a5833212f6f85c50ad Mon Sep 17 00:00:00 2001 From: leventov Date: Sat, 1 Oct 2016 20:09:27 +0300 Subject: [PATCH] Don't use synrhonized on a public object (the map itself) in ReplicatedChronicleMap.close() --- .../hash/impl/VanillaChronicleHash.java | 64 ++++++++++--------- .../chronicle/map/ReplicatedChronicleMap.java | 6 +- 2 files changed, 36 insertions(+), 34 deletions(-) diff --git a/src/main/java/net/openhft/chronicle/hash/impl/VanillaChronicleHash.java b/src/main/java/net/openhft/chronicle/hash/impl/VanillaChronicleHash.java index d5eaed471..803d1d957 100644 --- a/src/main/java/net/openhft/chronicle/hash/impl/VanillaChronicleHash.java +++ b/src/main/java/net/openhft/chronicle/hash/impl/VanillaChronicleHash.java @@ -638,43 +638,47 @@ public abstract class VanillaChronicleHash contextRef : allContexts) { - ChainingInterface context = contextRef.get(); - if (context != null && context.owner().isAlive()) { - // Ensures that if the thread owning this context will come to access - // chronicleHash concurrently with resource releasing operations below, it will - // fail due to the check in context.lockContextLocally() method. - // If the thread owning this context is currently accessing chronicleHash, - // closeContext() will spin-wait until the end of this access session. - context.closeContext(); - } - } - - bs.release(); - assert bs.refCount() == 0; - for (TierBulkData bulkData : tierBulkOffsets) { - if (bulkData.bytesStore != bs) { - bulkData.bytesStore.release(); - assert bulkData.bytesStore.refCount() == 0; - } - } - if (rafCleaner != null) - rafCleaner.clean(); + doClose(); } } + protected void doClose() { + // Synchronization on allContexts here in conjunction with checkOpen() in addContext() + // ensures threads using this chronicleHash for the first time concurrently with close() + // will not be able to access chronicleHash, or either will be visible during iteration + // over allContexts below + synchronized (allContexts) { + closed = true; + } + for (WeakReference contextRef : allContexts) { + ChainingInterface context = contextRef.get(); + if (context != null && context.owner().isAlive()) { + // Ensures that if the thread owning this context will come to access + // chronicleHash concurrently with resource releasing operations below, it will + // fail due to the check in context.lockContextLocally() method. + // If the thread owning this context is currently accessing chronicleHash, + // closeContext() will spin-wait until the end of this access session. + context.closeContext(); + } + } + + bs.release(); + assert bs.refCount() == 0; + for (TierBulkData bulkData : tierBulkOffsets) { + if (bulkData.bytesStore != bs) { + bulkData.bytesStore.release(); + assert bulkData.bytesStore.refCount() == 0; + } + } + if (rafCleaner != null) + rafCleaner.clean(); + } + @Override public boolean isOpen() { return !closed; diff --git a/src/main/java/net/openhft/chronicle/map/ReplicatedChronicleMap.java b/src/main/java/net/openhft/chronicle/map/ReplicatedChronicleMap.java index 83b0bc120..60c315e88 100644 --- a/src/main/java/net/openhft/chronicle/map/ReplicatedChronicleMap.java +++ b/src/main/java/net/openhft/chronicle/map/ReplicatedChronicleMap.java @@ -264,9 +264,7 @@ public class ReplicatedChronicleMap extends VanillaChronicleMap extends VanillaChronicleMap