From 551188eb167267c26fb3c3864f4cd4b38a79cdcb Mon Sep 17 00:00:00 2001 From: mobounya Date: Fri, 1 Mar 2024 20:21:28 +0100 Subject: [PATCH] Make operations on NamespacedHierarchicalStore fail if it's closed Track the active/closed state in NamespacedHierarchicalStore so it can throw an IllegalStateException when performing a modification or query on it after it has been already closed. This commit also makes close() an idempotent operation. Closes #3614 Closes #3800 --- .../release-notes-5.11.0-M2.adoc | 6 ++- .../store/NamespacedHierarchicalStore.java | 41 ++++++++++++++++++- .../NamespacedHierarchicalStoreTests.java | 37 +++++++++++++++++ 3 files changed, 81 insertions(+), 3 deletions(-) diff --git a/documentation/src/docs/asciidoc/release-notes/release-notes-5.11.0-M2.adoc b/documentation/src/docs/asciidoc/release-notes/release-notes-5.11.0-M2.adoc index a75015651c5..f8aaaafcecc 100644 --- a/documentation/src/docs/asciidoc/release-notes/release-notes-5.11.0-M2.adoc +++ b/documentation/src/docs/asciidoc/release-notes/release-notes-5.11.0-M2.adoc @@ -36,8 +36,10 @@ repository on GitHub. [[release-notes-5.11.0-M2-junit-platform-new-features-and-improvements]] ==== New Features and Improvements -* ❓ - +* `NamespacedHierarchicalStore` will now throw an IllegalStateException for + modification or query calls after it has been closed. Closing an already + closed store is a no-op. + - See link:https://github.com/junit-team/junit5/issues/3614[issue 3614] for details. [[release-notes-5.11.0-M2-junit-jupiter]] === JUnit Jupiter 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 18745c63019..cbcdc915e1c 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 = false; /** * Create a new store with the supplied parent. @@ -84,10 +85,11 @@ public NamespacedHierarchicalStore newChild() { * stored values in reverse insertion order. * *

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

Invocations of this method after the store has already been closed will be ignored. */ @Override public void close() { - if (this.closeAction == null) { + if (this.closeAction == null || this.closed) { return; } ThrowableCollector throwableCollector = new ThrowableCollector(__ -> false); @@ -97,6 +99,7 @@ public void close() { .sorted(EvaluatedValue.REVERSE_INSERT_ORDER) // .forEach(it -> throwableCollector.execute(() -> it.close(this.closeAction))); throwableCollector.assertEmpty(); + this.closed = true; } /** @@ -106,8 +109,12 @@ public void close() { * @param namespace the namespace; never {@code null} * @param key the key; never {@code null} * @return the stored value; may be {@code null} + * @throws IllegalStateException when querying from an already closed store */ public Object get(N namespace, Object key) { + if (this.closed) { + rejectQueryAfterClose(); + } StoredValue storedValue = getStoredValue(new CompositeKey<>(namespace, key)); return StoredValue.evaluateIfNotNull(storedValue); } @@ -122,8 +129,12 @@ public Object get(N namespace, Object key) { * @return the stored value; may be {@code null} * @throws NamespacedHierarchicalStoreException if the stored value cannot * be cast to the required type + * @throws IllegalStateException when querying from an already closed store */ public T get(N namespace, Object key, Class requiredType) throws NamespacedHierarchicalStoreException { + if (this.closed) { + rejectQueryAfterClose(); + } Object value = get(namespace, key); return castToRequiredType(key, value, requiredType); } @@ -137,8 +148,12 @@ public T get(N namespace, Object key, Class requiredType) throws Namespac * @param defaultCreator the function called with the supplied {@code key} * to create a new value; never {@code null} but may return {@code null} * @return the stored value; may be {@code null} + * @throws IllegalStateException when querying from an already closed store */ public Object getOrComputeIfAbsent(N namespace, K key, Function defaultCreator) { + if (this.closed) { + rejectQueryAfterClose(); + } Preconditions.notNull(defaultCreator, "defaultCreator must not be null"); CompositeKey compositeKey = new CompositeKey<>(namespace, key); StoredValue storedValue = getStoredValue(compositeKey); @@ -162,9 +177,13 @@ public Object getOrComputeIfAbsent(N namespace, K key, Function def * @return the stored value; may be {@code null} * @throws NamespacedHierarchicalStoreException if the stored value cannot * be cast to the required type + * @throws IllegalStateException when querying from an already closed store */ public V getOrComputeIfAbsent(N namespace, K key, Function defaultCreator, Class requiredType) throws NamespacedHierarchicalStoreException { + if (this.closed) { + rejectQueryAfterClose(); + } Object value = getOrComputeIfAbsent(namespace, key, defaultCreator); return castToRequiredType(key, value, requiredType); } @@ -182,8 +201,12 @@ public V getOrComputeIfAbsent(N namespace, K key, Function defaultC * @return the previously stored value; may be {@code null} * @throws NamespacedHierarchicalStoreException if the stored value cannot * be cast to the required type + * @throws IllegalStateException when modifying an already closed store */ public Object put(N namespace, Object key, Object value) throws NamespacedHierarchicalStoreException { + if (this.closed) { + rejectModificationAfterClose(); + } StoredValue oldValue = this.storedValues.put(new CompositeKey<>(namespace, key), storedValue(() -> value)); return StoredValue.evaluateIfNotNull(oldValue); } @@ -198,8 +221,12 @@ public Object put(N namespace, Object key, Object value) throws NamespacedHierar * @param namespace the namespace; never {@code null} * @param key the key; never {@code null} * @return the previously stored value; may be {@code null} + * @throws IllegalStateException when modifying an already closed store */ public Object remove(N namespace, Object key) { + if (this.closed) { + rejectModificationAfterClose(); + } StoredValue previous = this.storedValues.remove(new CompositeKey<>(namespace, key)); return StoredValue.evaluateIfNotNull(previous); } @@ -217,8 +244,12 @@ public Object remove(N namespace, Object key) { * @return the previously stored value; may be {@code null} * @throws NamespacedHierarchicalStoreException if the stored value cannot * be cast to the required type + * @throws IllegalStateException when modifying an already closed store */ public T remove(N namespace, Object key, Class requiredType) throws NamespacedHierarchicalStoreException { + if (this.closed) { + rejectModificationAfterClose(); + } Object value = remove(namespace, key); return castToRequiredType(key, value, requiredType); } @@ -256,6 +287,14 @@ private T castToRequiredType(Object key, Object value, Class requiredType requiredType.getName(), value.getClass().getName(), value)); } + private void rejectModificationAfterClose() { + throw new IllegalStateException("A NamespacedHierarchicalStore cannot be modified after it has been closed"); + } + + private void rejectQueryAfterClose() { + throw new IllegalStateException("A NamespacedHierarchicalStore cannot be queried after it has been closed"); + } + 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 551bff7e4e4..b8ace58e7b6 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 closeIsIdempotent() 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 rejectsModificationAfterClose() { + 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 rejectsQueryAfterClose() { + 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 static Object createObject(String display) {