Fix FunctionAroundWrapper state and wrapped-function cache eviction

Reset the 'wrapped' flag in a finally block in
FunctionInvocationWrapper.apply(). Previously it stayed true after a
call with non-Message or Publisher input, so later Message calls on the
same cached function skipped the around-advice (e.g. tracing).

Make the wrapped-function cache access-ordered so that eviction is
least-recently-used instead of first-in-first-out, and correct the
copy-pasted eviction debug message.

Add regression tests for both.

Signed-off-by: Oleg Zhurakousky <oleg.zhurakousky@broadcom.com>

Author: olegz

spring-cloud-function-context/src/main/java/org/springframework/cloud/function/context/catalog/SimpleFunctionRegistry.java

@@ -135,13 +135,14 @@ public SimpleFunctionRegistry(ConversionService conversionService, CompositeMess
 		this.messageConverter = messageConverter;
 		this.functionInvocationHelper = functionInvocationHelper;
 		this.functionProperties = functionProperties;
-		this.wrappedFunctionDefinitions = new LinkedHashMap<>() {
+		// access-ordered so that eviction is least-recently-used rather than first-in-first-out
+		this.wrappedFunctionDefinitions = new LinkedHashMap<>(16, 0.75f, true) {
 			@Override
 			protected boolean removeEldestEntry(Map.Entry<String, FunctionInvocationWrapper> eldest) {
 				boolean remove = size() > wrappedFunctionDefinitionsCacheSize;
 				if (remove) {
 					if (logger.isDebugEnabled()) {
-						logger.debug("Removing message channel from cache " + eldest.getKey());
+						logger.debug("Evicting wrapped function from cache: " + eldest.getKey());
 					}
 				}
 				return remove;
@@ -662,7 +663,12 @@ public Object apply(Object input) {
 			Object result;
 			if (functionAroundWrapper != null && !this.wrapped) {
 				this.wrapped = true;
-				result = functionAroundWrapper.apply(input, this);
+				try {
+					result = functionAroundWrapper.apply(input, this);
+				}
+				finally {
+					this.wrapped = false;
+				}
 			}
 			else {
 				result = this.doApply(input);

spring-cloud-function-context/src/test/java/org/springframework/cloud/function/context/catalog/SimpleFunctionRegistryTests.java

@@ -80,6 +80,7 @@
 import org.springframework.messaging.converter.ProtobufMessageConverter;
 import org.springframework.messaging.converter.StringMessageConverter;
 import org.springframework.messaging.support.MessageBuilder;
+import org.springframework.test.util.ReflectionTestUtils;
 import org.springframework.util.MimeType;
 import org.springframework.util.ReflectionUtils;
 
@@ -110,6 +111,49 @@ public void before() {
 		this.conversionService = new DefaultConversionService();
 	}
 
+	@Test
+	public void testAroundWrapperAppliedAfterNonMessageInvocation() {
+		SimpleFunctionRegistry catalog = new SimpleFunctionRegistry(this.conversionService, this.messageConverter,
+				new GsonMapper(new Gson()));
+		AtomicInteger aroundInvocations = new AtomicInteger();
+		ReflectionTestUtils.setField(catalog, "functionAroundWrapper", new FunctionAroundWrapper() {
+			@Override
+			protected Object doApply(Object input, FunctionInvocationWrapper targetFunction) {
+				aroundInvocations.incrementAndGet();
+				return targetFunction.apply(input);
+			}
+		});
+		Function<String, String> uppercase = String::toUpperCase;
+		catalog.register(new FunctionRegistration<>(uppercase, "uppercase")
+				.type(ResolvableType.forClassWithGenerics(Function.class, String.class, String.class).getType()));
+		FunctionInvocationWrapper function = catalog.lookup("uppercase");
+
+		// non-Message input is not advised, but must not leave the wrapper stuck in 'wrapped' state
+		function.apply("a");
+		assertThat(aroundInvocations.get()).isEqualTo(0);
+
+		function.apply(MessageBuilder.withPayload("b").build());
+		assertThat(aroundInvocations.get()).isEqualTo(1);
+		function.apply(MessageBuilder.withPayload("c").build());
+		assertThat(aroundInvocations.get()).isEqualTo(2);
+	}
+
+	@Test
+	public void testWrappedFunctionCacheEvictsLeastRecentlyUsed() {
+		SimpleFunctionRegistry catalog = new SimpleFunctionRegistry(this.conversionService, this.messageConverter,
+				new GsonMapper(new Gson()), null, null, 2);
+		Type type = ResolvableType.forClassWithGenerics(Function.class, String.class, String.class).getType();
+		for (String name : new String[] {"a", "b", "c"}) {
+			Function<String, String> f = v -> v;
+			catalog.register(new FunctionRegistration<>(f, name).type(type));
+		}
+		Object a = catalog.lookup("a");
+		catalog.lookup("b");
+		assertThat((Object) catalog.lookup("a")).isSameAs(a); // 'a' is now most recently used
+		catalog.lookup("c"); // exceeds cache size; least recently used ('b') must be evicted
+		assertThat((Object) catalog.lookup("a")).isSameAs(a);
+	}
+
 	@ParameterizedTest
 	@ValueSource(strings = {
 		"aaaaaaaaaa", // no problem