Close resource in inverse order of addition (#2387)

Resolves #2211.

documentation/src/docs/asciidoc/release-notes/release-notes-5.7.0.adoc

@@ -31,7 +31,8 @@ on GitHub.
 
 ==== Bug Fixes
 
-* ❓
+* `CloseableResource` instances stored in `ExtensionContext.Store` are now closed in the
+  reverse order they were added in. Previously, the order was undefined and unstable.
 
 ==== Deprecations and Breaking Changes
 

documentation/src/docs/asciidoc/user-guide/extensions.adoc

@@ -569,8 +569,8 @@ methods available for storing and retrieving values via the `{ExtensionContext_S
 .`ExtensionContext.Store.CloseableResource`
 NOTE: An extension context store is bound to its extension context lifecycle. When an
 extension context lifecycle ends it closes its associated store. All stored values
-that are instances of `CloseableResource` are notified by
-an invocation of their `close()` method.
+that are instances of `CloseableResource` are notified by an invocation of their `close()`
+method in the inverse order they were added in.
 
 [[extensions-supported-utilities]]
 === Supported Utilities in Extensions

junit-jupiter-api/src/main/java/org/junit/jupiter/api/extension/ExtensionContext.java

@@ -396,6 +396,9 @@ interface Store {
 		 * <p>Note that the {@code CloseableResource} API is only honored for
 		 * objects stored within an extension context {@link Store Store}.
 		 *
+		 * <p>The resources stored in a {@link Store Store} are closed in the
+		 * inverse order they were added in.
+		 *
 		 * @since 5.1
 		 */
 		@API(status = STABLE, since = "5.1")

junit-jupiter-api/src/testFixtures/java/org/junit/jupiter/api/extension/ExtensionContextParameterResolver.java

@@ -0,0 +1,26 @@
+/*
+ * Copyright 2015-2020 the original author or authors.
+ *
+ * All rights reserved. This program and the accompanying materials are
+ * made available under the terms of the Eclipse Public License v2.0 which
+ * accompanies this distribution and is available at
+ *
+ * https://www.eclipse.org/legal/epl-v20.html
+ */
+
+package org.junit.jupiter.api.extension;
+
+public class ExtensionContextParameterResolver implements ParameterResolver {
+
+	@Override
+	public boolean supportsParameter(ParameterContext parameterContext, ExtensionContext extensionContext)
+			throws ParameterResolutionException {
+		return ExtensionContext.class.equals(parameterContext.getParameter().getType());
+	}
+
+	@Override
+	public Object resolveParameter(ParameterContext parameterContext, ExtensionContext extensionContext)
+			throws ParameterResolutionException {
+		return extensionContext;
+	}
+}

junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/execution/ExtensionValuesStore.java

@@ -15,10 +15,12 @@
 import static org.junit.platform.commons.util.ReflectionUtils.getWrapperType;
 import static org.junit.platform.commons.util.ReflectionUtils.isAssignableTo;
 
+import java.util.Comparator;
 import java.util.Objects;
 import java.util.Optional;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ConcurrentMap;
+import java.util.concurrent.atomic.AtomicInteger;
 import java.util.concurrent.locks.Lock;
 import java.util.concurrent.locks.ReentrantLock;
 import java.util.function.Function;
@@ -27,6 +29,7 @@
 import org.apiguardian.api.API;
 import org.junit.jupiter.api.extension.ExtensionContext;
 import org.junit.jupiter.api.extension.ExtensionContext.Namespace;
+import org.junit.jupiter.api.extension.ExtensionContext.Store.CloseableResource;
 import org.junit.jupiter.api.extension.ExtensionContextException;
 import org.junit.platform.engine.support.hierarchical.ThrowableCollector;
 
@@ -39,34 +42,36 @@
 @API(status = INTERNAL, since = "5.0")
 public class ExtensionValuesStore {
 
+	private static final Comparator<StoredValue> REVERSE_INSERT_ORDER = Comparator.<StoredValue, Integer> comparing(
+		it -> it.order).reversed();
+
+	private final AtomicInteger insertOrderSequence = new AtomicInteger();
+	private final ConcurrentMap<CompositeKey, StoredValue> storedValues = new ConcurrentHashMap<>(4);
 	private final ExtensionValuesStore parentStore;
-	private final ConcurrentMap<CompositeKey, Supplier<Object>> storedValues = new ConcurrentHashMap<>(4);
 
 	public ExtensionValuesStore(ExtensionValuesStore parentStore) {
 		this.parentStore = parentStore;
 	}
 
 	/**
-	 * Close all values that implement {@link ExtensionContext.Store.CloseableResource}.
+	 * Close all values that implement {@link CloseableResource}.
 	 *
 	 * @implNote Only close values stored in this instance. This implementation
 	 * does not close values in parent stores.
 	 */
 	public void closeAllStoredCloseableValues() {
 		ThrowableCollector throwableCollector = createThrowableCollector();
-		for (Supplier<Object> supplier : storedValues.values()) {
-			Object value = supplier.get();
-			if (value instanceof ExtensionContext.Store.CloseableResource) {
-				ExtensionContext.Store.CloseableResource resource = (ExtensionContext.Store.CloseableResource) value;
-				throwableCollector.execute(resource::close);
-			}
-		}
+		storedValues.values().stream() //
+				.filter(storedValue -> storedValue.evaluate() instanceof CloseableResource) //
+				.sorted(REVERSE_INSERT_ORDER) //
+				.map(storedValue -> (CloseableResource) storedValue.evaluate()) //
+				.forEach(resource -> throwableCollector.execute(resource::close));
 		throwableCollector.assertEmpty();
 	}
 
 	Object get(Namespace namespace, Object key) {
-		Supplier<Object> storedValue = getStoredValue(new CompositeKey(namespace, key));
-		return (storedValue != null ? storedValue.get() : null);
+		StoredValue storedValue = getStoredValue(new CompositeKey(namespace, key));
+		return (storedValue != null ? storedValue.evaluate() : null);
 	}
 
 	<T> T get(Namespace namespace, Object key, Class<T> requiredType) {
@@ -76,12 +81,12 @@ <T> T get(Namespace namespace, Object key, Class<T> requiredType) {
 
 	<K, V> Object getOrComputeIfAbsent(Namespace namespace, K key, Function<K, V> defaultCreator) {
 		CompositeKey compositeKey = new CompositeKey(namespace, key);
-		Supplier<Object> storedValue = getStoredValue(compositeKey);
+		StoredValue storedValue = getStoredValue(compositeKey);
 		if (storedValue == null) {
-			Supplier<Object> newValue = new MemoizingSupplier(() -> defaultCreator.apply(key));
+			StoredValue newValue = storedValue(new MemoizingSupplier(() -> defaultCreator.apply(key)));
 			storedValue = Optional.ofNullable(storedValues.putIfAbsent(compositeKey, newValue)).orElse(newValue);
 		}
-		return storedValue.get();
+		return storedValue.evaluate();
 	}
 
 	<K, V> V getOrComputeIfAbsent(Namespace namespace, K key, Function<K, V> defaultCreator, Class<V> requiredType) {
@@ -90,30 +95,32 @@ <K, V> V getOrComputeIfAbsent(Namespace namespace, K key, Function<K, V> default
 	}
 
 	void put(Namespace namespace, Object key, Object value) {
-		storedValues.put(new CompositeKey(namespace, key), () -> value);
+		storedValues.put(new CompositeKey(namespace, key), storedValue(() -> value));
+	}
+
+	private StoredValue storedValue(Supplier<Object> value) {
+		return new StoredValue(insertOrderSequence.getAndIncrement(), value);
 	}
 
 	Object remove(Namespace namespace, Object key) {
-		Supplier<Object> previous = storedValues.remove(new CompositeKey(namespace, key));
-		return (previous != null ? previous.get() : null);
+		StoredValue previous = storedValues.remove(new CompositeKey(namespace, key));
+		return (previous != null ? previous.evaluate() : null);
 	}
 
 	<T> T remove(Namespace namespace, Object key, Class<T> requiredType) {
 		Object value = remove(namespace, key);
 		return castToRequiredType(key, value, requiredType);
 	}
 
-	private Supplier<Object> getStoredValue(CompositeKey compositeKey) {
-		Supplier<Object> storedValue = storedValues.get(compositeKey);
+	private StoredValue getStoredValue(CompositeKey compositeKey) {
+		StoredValue storedValue = storedValues.get(compositeKey);
 		if (storedValue != null) {
 			return storedValue;
 		}
-		else if (parentStore != null) {
+		if (parentStore != null) {
 			return parentStore.getStoredValue(compositeKey);
 		}
-		else {
-			return null;
-		}
+		return null;
 	}
 
 	@SuppressWarnings("unchecked")
@@ -161,6 +168,22 @@ public int hashCode() {
 
 	}
 
+	private static class StoredValue {
+
+		private final int order;
+		private final Supplier<Object> supplier;
+
+		public StoredValue(int order, Supplier<Object> supplier) {
+			this.order = order;
+			this.supplier = supplier;
+		}
+
+		private Object evaluate() {
+			return supplier.get();
+		}
+
+	}
+
 	private static class MemoizingSupplier implements Supplier<Object> {
 
 		private static final Object NO_VALUE_SET = new Object();

junit-jupiter-engine/src/test/java/org/junit/jupiter/api/extension/CloseableResourceIntegrationTests.java

@@ -0,0 +1,48 @@
+/*
+ * Copyright 2015-2020 the original author or authors.
+ *
+ * All rights reserved. This program and the accompanying materials are
+ * made available under the terms of the Eclipse Public License v2.0 which
+ * accompanies this distribution and is available at
+ *
+ * https://www.eclipse.org/legal/epl-v20.html
+ */
+
+package org.junit.jupiter.api.extension;
+
+import static org.junit.platform.testkit.engine.EventConditions.reportEntry;
+
+import java.util.Map;
+
+import org.jetbrains.annotations.NotNull;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.engine.AbstractJupiterTestEngineTests;
+
+public class CloseableResourceIntegrationTests extends AbstractJupiterTestEngineTests {
+
+	@Test
+	void closesCloseableResourcesInReverseInsertOrder() {
+		executeTestsForClass(TestCase.class).allEvents().reportingEntryPublished() //
+				.assertEventsMatchExactly( //
+					reportEntry(Map.of("3", "closed")), //
+					reportEntry(Map.of("2", "closed")), //
+					reportEntry(Map.of("1", "closed")));
+	}
+
+	@ExtendWith(ExtensionContextParameterResolver.class)
+	static class TestCase {
+		@Test
+		void closesCloseableResourcesInExtensionContext(ExtensionContext extensionContext) {
+			ExtensionContext.Store store = extensionContext.getStore(ExtensionContext.Namespace.GLOBAL);
+			store.put("foo", reportEntryOnClose(extensionContext, "1"));
+			store.put("bar", reportEntryOnClose(extensionContext, "2"));
+			store.put("baz", reportEntryOnClose(extensionContext, "3"));
+		}
+
+		@NotNull
+		private ExtensionContext.Store.CloseableResource reportEntryOnClose(ExtensionContext extensionContext,
+				String key) {
+			return () -> extensionContext.publishReportEntry(Map.of(key, "closed"));
+		}
+	}
+}

junit-jupiter-engine/src/test/java/org/junit/jupiter/engine/execution/ExtensionValuesStoreTests.java

@@ -378,7 +378,7 @@ void orderOfNamespacePartsDoesMatter() {
 		}
 
 		@Test
-		void additionNamespacePartMakesADifferenc() {
+		void additionNamespacePartMakesADifference() {
 
 			Namespace ns1 = Namespace.create("part1", "part2");
 			Namespace ns2 = Namespace.create("part1");

junit-jupiter-engine/src/test/java/org/junit/jupiter/engine/extension/ExtensionContextExecutionTests.java

@@ -21,9 +21,7 @@
 import org.junit.jupiter.api.extension.BeforeAllCallback;
 import org.junit.jupiter.api.extension.ExtendWith;
 import org.junit.jupiter.api.extension.ExtensionContext;
-import org.junit.jupiter.api.extension.ParameterContext;
-import org.junit.jupiter.api.extension.ParameterResolutionException;
-import org.junit.jupiter.api.extension.ParameterResolver;
+import org.junit.jupiter.api.extension.ExtensionContextParameterResolver;
 import org.junit.jupiter.engine.AbstractJupiterTestEngineTests;
 
 class ExtensionContextExecutionTests extends AbstractJupiterTestEngineTests {
@@ -45,20 +43,6 @@ void extensionContextHierarchy(ExtensionContext methodExtensionContext) {
 		assertThat(engineExtensionContext.orElse(null).getParent()).isEmpty();
 	}
 
-	static class ExtensionContextParameterResolver implements ParameterResolver {
-		@Override
-		public boolean supportsParameter(ParameterContext parameterContext, ExtensionContext extensionContext)
-				throws ParameterResolutionException {
-			return ExtensionContext.class.equals(parameterContext.getParameter().getType());
-		}
-
-		@Override
-		public Object resolveParameter(ParameterContext parameterContext, ExtensionContext extensionContext)
-				throws ParameterResolutionException {
-			return extensionContext;
-		}
-	}
-
 	@Test
 	void twoTestClassesCanShareStateViaEngineExtensionContext() {
 		Parent.counter.set(0);

junit-platform-testkit/src/main/java/org/junit/platform/testkit/engine/EventConditions.java

@@ -12,6 +12,7 @@
 
 import static java.util.function.Predicate.isEqual;
 import static java.util.stream.Collectors.toList;
+import static org.apiguardian.api.API.Status.EXPERIMENTAL;
 import static org.apiguardian.api.API.Status.MAINTAINED;
 import static org.assertj.core.api.Assertions.allOf;
 import static org.junit.platform.commons.util.FunctionUtils.where;
@@ -29,6 +30,7 @@
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.List;
+import java.util.Map;
 import java.util.function.Predicate;
 
 import org.apiguardian.api.API;
@@ -40,6 +42,7 @@
 import org.junit.platform.engine.TestExecutionResult;
 import org.junit.platform.engine.TestExecutionResult.Status;
 import org.junit.platform.engine.UniqueId;
+import org.junit.platform.engine.reporting.ReportEntry;
 import org.junit.platform.engine.support.descriptor.EngineDescriptor;
 
 /**
@@ -411,4 +414,15 @@ public static Condition<Event> reason(Predicate<String> predicate) {
 		return new Condition<>(byPayload(String.class, predicate), "event with custom reason predicate");
 	}
 
+	/**
+	 * Create a new {@link Condition} that matches if and only if an
+	 * {@link Event}'s {@linkplain Event#getPayload() payload} is an instance of
+	 * {@link ReportEntry} that contains the supplied key-value pairs.
+	 */
+	@API(status = EXPERIMENTAL, since = "1.7")
+	public static Condition<Event> reportEntry(Map<String, String> keyValuePairs) {
+		return new Condition<>(byPayload(ReportEntry.class, it -> it.getKeyValuePairs().equals(keyValuePairs)),
+			"event for report entry with key-value pairs %s", keyValuePairs);
+	}
+
 }