From 9976b4ea67ebb22368e5c1b81082f4e23c19b631 Mon Sep 17 00:00:00 2001 From: mobounya Date: Fri, 1 Mar 2024 20:21:28 +0100 Subject: [PATCH] Make NamespacedHierarchicalStore ops fail if closed Track the active/closed state in NamespacedHierarchicalStore so it can throw an IllegalStateException when doing a modification/query on it after it has been already closed. Make a close operation on an already closed store a no-op. Issue: #3614 --- .../store/NamespacedHierarchicalStore.java | 31 +++++++++++++++- .../NamespacedHierarchicalStoreTests.java | 37 +++++++++++++++++++ 2 files changed, 67 insertions(+), 1 deletion(-) diff --git a/junit-platform-engine/src/main/java/org/junit/platform/engine/support/store/NamespacedHierarchicalStore.java b/junit-platform-engine/src/main/java/org/junit/platform/engine/support/store/NamespacedHierarchicalStore.java index 18745c63019d..edb9dbe3d9d4 100644 --- a/junit-platform-engine/src/main/java/org/junit/platform/engine/support/store/NamespacedHierarchicalStore.java +++ b/junit-platform-engine/src/main/java/org/junit/platform/engine/support/store/NamespacedHierarchicalStore.java @@ -49,6 +49,7 @@ public final class NamespacedHierarchicalStore implements AutoCloseable { private final ConcurrentMap, StoredValue> storedValues = new ConcurrentHashMap<>(4); private final NamespacedHierarchicalStore parentStore; private final CloseAction closeAction; + private boolean closed; /** * Create a new store with the supplied parent. @@ -57,6 +58,7 @@ public final class NamespacedHierarchicalStore implements AutoCloseable { */ public NamespacedHierarchicalStore(NamespacedHierarchicalStore parentStore) { this(parentStore, null); + this.closed = false; } /** @@ -69,6 +71,7 @@ public NamespacedHierarchicalStore(NamespacedHierarchicalStore parentStore) { public NamespacedHierarchicalStore(NamespacedHierarchicalStore parentStore, CloseAction closeAction) { this.parentStore = parentStore; this.closeAction = closeAction; + this.closed = false; } /** @@ -84,10 +87,13 @@ public NamespacedHierarchicalStore newChild() { * stored values in reverse insertion order. * *

Closing a store does not close its parent or any of its children. + *

Closing an already closed store is a no-op. + * @throws IllegalStateException if Modifying or querying from an already + * closed store. */ @Override public void close() { - if (this.closeAction == null) { + if (this.closeAction == null || this.closed) { return; } ThrowableCollector throwableCollector = new ThrowableCollector(__ -> false); @@ -97,6 +103,7 @@ public void close() { .sorted(EvaluatedValue.REVERSE_INSERT_ORDER) // .forEach(it -> throwableCollector.execute(() -> it.close(this.closeAction))); throwableCollector.assertEmpty(); + this.closed = true; } /** @@ -108,6 +115,8 @@ public void close() { * @return the stored value; may be {@code null} */ public Object get(N namespace, Object key) { + if (this.closed) + illegalQueryAfterClose(); StoredValue storedValue = getStoredValue(new CompositeKey<>(namespace, key)); return StoredValue.evaluateIfNotNull(storedValue); } @@ -124,6 +133,8 @@ public Object get(N namespace, Object key) { * be cast to the required type */ public T get(N namespace, Object key, Class requiredType) throws NamespacedHierarchicalStoreException { + if (this.closed) + illegalQueryAfterClose(); Object value = get(namespace, key); return castToRequiredType(key, value, requiredType); } @@ -139,6 +150,8 @@ public T get(N namespace, Object key, Class requiredType) throws Namespac * @return the stored value; may be {@code null} */ public Object getOrComputeIfAbsent(N namespace, K key, Function defaultCreator) { + if (this.closed) + illegalQueryAfterClose(); Preconditions.notNull(defaultCreator, "defaultCreator must not be null"); CompositeKey compositeKey = new CompositeKey<>(namespace, key); StoredValue storedValue = getStoredValue(compositeKey); @@ -165,6 +178,8 @@ public Object getOrComputeIfAbsent(N namespace, K key, Function def */ public V getOrComputeIfAbsent(N namespace, K key, Function defaultCreator, Class requiredType) throws NamespacedHierarchicalStoreException { + if (this.closed) + illegalQueryAfterClose(); Object value = getOrComputeIfAbsent(namespace, key, defaultCreator); return castToRequiredType(key, value, requiredType); } @@ -184,6 +199,8 @@ public V getOrComputeIfAbsent(N namespace, K key, Function defaultC * be cast to the required type */ public Object put(N namespace, Object key, Object value) throws NamespacedHierarchicalStoreException { + if (this.closed) + illegalModifyAfterClose(); StoredValue oldValue = this.storedValues.put(new CompositeKey<>(namespace, key), storedValue(() -> value)); return StoredValue.evaluateIfNotNull(oldValue); } @@ -200,6 +217,8 @@ public Object put(N namespace, Object key, Object value) throws NamespacedHierar * @return the previously stored value; may be {@code null} */ public Object remove(N namespace, Object key) { + if (this.closed) + illegalModifyAfterClose(); StoredValue previous = this.storedValues.remove(new CompositeKey<>(namespace, key)); return StoredValue.evaluateIfNotNull(previous); } @@ -219,6 +238,8 @@ public Object remove(N namespace, Object key) { * be cast to the required type */ public T remove(N namespace, Object key, Class requiredType) throws NamespacedHierarchicalStoreException { + if (this.closed) + illegalModifyAfterClose(); Object value = remove(namespace, key); return castToRequiredType(key, value, requiredType); } @@ -256,6 +277,14 @@ private T castToRequiredType(Object key, Object value, Class requiredType requiredType.getName(), value.getClass().getName(), value)); } + private void illegalModifyAfterClose() { + throw new IllegalStateException("Modifying a closed resource"); + } + + private void illegalQueryAfterClose() { + throw new IllegalStateException("Modifying a closed resource"); + } + private static class CompositeKey { private final N namespace; diff --git a/platform-tests/src/test/java/org/junit/platform/engine/support/store/NamespacedHierarchicalStoreTests.java b/platform-tests/src/test/java/org/junit/platform/engine/support/store/NamespacedHierarchicalStoreTests.java index c4c87785fe4f..10695f418ac9 100644 --- a/platform-tests/src/test/java/org/junit/platform/engine/support/store/NamespacedHierarchicalStoreTests.java +++ b/platform-tests/src/test/java/org/junit/platform/engine/support/store/NamespacedHierarchicalStoreTests.java @@ -19,6 +19,7 @@ import static org.junit.platform.commons.test.ConcurrencyTestingUtils.executeConcurrently; import static org.mockito.Mockito.inOrder; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoInteractions; import static org.mockito.Mockito.verifyNoMoreInteractions; @@ -435,6 +436,42 @@ void doesNotIgnoreStoredValuesThatThrewUnrecoverableFailuresDuringCleanup() { verifyNoInteractions(closeAction); } + + @Test + void testNoOpAfterClose() throws Throwable { + store.put(namespace, "key1", "value1"); + + verifyNoInteractions(closeAction); + + store.close(); + + verify(closeAction, times(1)).close(namespace, "key1", "value1"); + + store.close(); + + verifyNoMoreInteractions(closeAction); + } + + @Test + void throwExceptionForModifyingAfterClose() { + store.close(); + + assertThrows(IllegalStateException.class, () -> store.put(namespace, "key1", "value1")); + assertThrows(IllegalStateException.class, () -> store.remove(namespace, "key1")); + assertThrows(IllegalStateException.class, () -> store.remove(namespace, "key1", Number.class)); + } + + @Test + void throwExceptionForQueryingAfterClose() { + store.close(); + + assertThrows(IllegalStateException.class, () -> store.get(namespace, "key1")); + assertThrows(IllegalStateException.class, () -> store.get(namespace, "key1", Integer.class)); + assertThrows(IllegalStateException.class, + () -> store.getOrComputeIfAbsent(namespace, "key1", k -> "tzadina 9ba7")); + assertThrows(IllegalStateException.class, + () -> store.getOrComputeIfAbsent(namespace, "key1", k -> 1337, Integer.class)); + } } private Object createObject(final String display) {