Skip to content

Commit

Permalink
Make NamespacedHierarchicalStore ops fail if closed
Browse files Browse the repository at this point in the history 
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: junit-team#3614
  • Loading branch information
@mobounya
mobounya committed Apr 29, 2024
1 parent d1a4d03 commit a49e575
Show file tree
Hide file tree
Showing 2 changed files with 77 additions and 1 deletion.
41 changes: 40 additions & 1 deletion ...ne/src/main/java/org/junit/platform/engine/support/store/NamespacedHierarchicalStore.java
View file
Expand Up @@ -49,6 +49,7 @@ public final class NamespacedHierarchicalStore<N> implements AutoCloseable {
private final ConcurrentMap<CompositeKey<N>, StoredValue> storedValues = new ConcurrentHashMap<>(4);
private final NamespacedHierarchicalStore<N> parentStore;
private final CloseAction<N> closeAction;
private boolean closed = false;

/**
* Create a new store with the supplied parent.
Expand Down Expand Up @@ -84,10 +85,11 @@ public NamespacedHierarchicalStore<N> newChild() {
* stored values in reverse insertion order.
*
* <p>Closing a store does not close its parent or any of its children.
* <p>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);
Expand All @@ -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;
}

/**
Expand All @@ -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);
}
Expand All @@ -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> T get(N namespace, Object key, Class<T> requiredType) throws NamespacedHierarchicalStoreException {
if (this.closed) {
rejectQueryAfterClose();
}
Object value = get(namespace, key);
return castToRequiredType(key, value, requiredType);
}
Expand All @@ -137,8 +148,12 @@ public <T> T get(N namespace, Object key, Class<T> 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 <K, V> Object getOrComputeIfAbsent(N namespace, K key, Function<K, V> defaultCreator) {
if (this.closed) {
rejectQueryAfterClose();
}
Preconditions.notNull(defaultCreator, "defaultCreator must not be null");
CompositeKey<N> compositeKey = new CompositeKey<>(namespace, key);
StoredValue storedValue = getStoredValue(compositeKey);
Expand All @@ -162,9 +177,13 @@ public <K, V> Object getOrComputeIfAbsent(N namespace, K key, Function<K, V> 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 <K, V> V getOrComputeIfAbsent(N namespace, K key, Function<K, V> defaultCreator, Class<V> requiredType)
throws NamespacedHierarchicalStoreException {
if (this.closed) {
rejectQueryAfterClose();
}
Object value = getOrComputeIfAbsent(namespace, key, defaultCreator);
return castToRequiredType(key, value, requiredType);
}
Expand All @@ -182,8 +201,12 @@ public <K, V> V getOrComputeIfAbsent(N namespace, K key, Function<K, V> 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);
}
Expand All @@ -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);
}
Expand All @@ -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> T remove(N namespace, Object key, Class<T> requiredType) throws NamespacedHierarchicalStoreException {
if (this.closed) {
rejectModificationAfterClose();
}
Object value = remove(namespace, key);
return castToRequiredType(key, value, requiredType);
}
Expand Down Expand Up @@ -256,6 +287,14 @@ private <T> T castToRequiredType(Object key, Object value, Class<T> 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<N> {

private final N namespace;
Expand Down
37 changes: 37 additions & 0 deletions ...c/test/java/org/junit/platform/engine/support/store/NamespacedHierarchicalStoreTests.java
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -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 Object createObject(final String display) {
Expand Down

0 comments on commit a49e575

Please sign in to comment.