From 9d5831a899429f8954dbc59f354f945a500aa3fe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Basl=C3=A9?= Date: Fri, 3 Feb 2023 11:55:05 +0100 Subject: [PATCH 01/19] Support `@Scheduled` fixedDelay/fixedRate on Publisher-returning methods This commit adds support for `@Scheduled` annotation on reactive methods in fixed delay or fixed rate mode. Cron mode is not supported. Reactive methods are methods that return a Publisher or a subclass of Publisher. This is only considered if Reactor (and Reactive Streams) is present at runtime. This is implemented using Reactor operators, as a `Flux` that repeatedly flatmaps to the `Publisher` in the background, re-subscribing to it according to the `@Scheduled` configuration. The method which creates the `Publisher` is only called once. If the `Publisher` errors, the exception is logged at warn level and otherwise ignored. As a result the underlying `Flux` will continue re-subscribing to the `Publisher`. Note that if Reactor is not present, the standard processing applies and the method itself will repeatedly be scheduled for execution, ignoring the returned `Publisher` and not even subscribing to it. This effectively becomes a no-op operation unless the method has other side effects. Closes gh-23533 --- spring-context/spring-context.gradle | 1 + .../ScheduledAnnotationBeanPostProcessor.java | 153 +++++++++++++++++- .../ScheduledAnnotationReactiveSupport.java | 140 ++++++++++++++++ 3 files changed, 292 insertions(+), 2 deletions(-) create mode 100644 spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupport.java diff --git a/spring-context/spring-context.gradle b/spring-context/spring-context.gradle index 754bfcb72fe4..86e14c454dd2 100644 --- a/spring-context/spring-context.gradle +++ b/spring-context/spring-context.gradle @@ -27,6 +27,7 @@ dependencies { optional("org.jetbrains.kotlin:kotlin-reflect") optional("org.jetbrains.kotlin:kotlin-stdlib") optional("org.reactivestreams:reactive-streams") + optional("io.projectreactor:reactor-core") testImplementation(project(":spring-core-test")) testImplementation(testFixtures(project(":spring-aop"))) testImplementation(testFixtures(project(":spring-beans"))) diff --git a/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationBeanPostProcessor.java b/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationBeanPostProcessor.java index 14272d35e776..cff077325a25 100644 --- a/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationBeanPostProcessor.java +++ b/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationBeanPostProcessor.java @@ -98,6 +98,7 @@ * @author Elizabeth Chatman * @author Victor Brown * @author Sam Brannen + * @author Simon Baslé * @since 3.0 * @see Scheduled * @see EnableScheduling @@ -142,6 +143,7 @@ public class ScheduledAnnotationBeanPostProcessor private final Set> nonAnnotatedClasses = Collections.newSetFromMap(new ConcurrentHashMap<>(64)); private final Map> scheduledTasks = new IdentityHashMap<>(16); + private final Map> scheduledReactiveTasks = new IdentityHashMap<>(16); /** @@ -316,6 +318,15 @@ private void finishRegistration() { } this.registrar.afterPropertiesSet(); + // Start the reactive tasks (we synchronize on the common scheduledTasks on purpose) + synchronized (this.scheduledTasks) { + for (Set reactiveTasks : this.scheduledReactiveTasks.values()) { + for (ScheduledAnnotationReactiveSupport.ReactiveTask reactiveTask : reactiveTasks) { + reactiveTask.subscribe(); + } + } + } + } private T resolveSchedulerBean(BeanFactory beanFactory, Class schedulerType, boolean byName) { @@ -384,6 +395,17 @@ public Object postProcessAfterInitialization(Object bean, String beanName) { return bean; } + protected void processScheduled(Scheduled scheduled, Method method, Object bean) { + // If Reactor is on the classpath, check for methods that return a Publisher + if (ScheduledAnnotationReactiveSupport.reactorPresent) { + if (ScheduledAnnotationReactiveSupport.isReactive(method)) { + processScheduledReactive(scheduled, method, bean); + return; + } + } + processScheduledSync(scheduled, method, bean); + } + /** * Process the given {@code @Scheduled} method declaration on the given bean. * @param scheduled the {@code @Scheduled} annotation @@ -391,7 +413,7 @@ public Object postProcessAfterInitialization(Object bean, String beanName) { * @param bean the target bean instance * @see #createRunnable(Object, Method) */ - protected void processScheduled(Scheduled scheduled, Method method, Object bean) { + protected void processScheduledSync(Scheduled scheduled, Method method, Object bean) { try { Runnable runnable = createRunnable(bean, method); boolean processedSchedule = false; @@ -516,6 +538,118 @@ protected void processScheduled(Scheduled scheduled, Method method, Object bean) } } + /** + * Process the given {@code @Scheduled} method declaration on the given bean which + * returns a {@code Publisher}, repeatedly subscribing to the returned publisher + * according to the fixedDelay/fixedRate configuration. Cron configuration isn't supported, + * nor is non-Publisher return types (even if a {@code ReactiveAdapter} is registered). + * @param scheduled the {@code @Scheduled} annotation + * @param method the method that the annotation has been declared on, which MUST return a Publisher + * @param bean the target bean instance + * @see #createRunnable(Object, Method) + */ + protected void processScheduledReactive(Scheduled scheduled, Method method, Object bean) { + try { + boolean processedSchedule = false; + String errorMessage = + "Exactly one of the 'fixedDelay(String)' or 'fixedRate(String)' attributes is required"; + + Set reactiveTasks = new LinkedHashSet<>(4); + + // Determine initial delay + Duration initialDelay = toDuration(scheduled.initialDelay(), scheduled.timeUnit()); + String initialDelayString = scheduled.initialDelayString(); + if (StringUtils.hasText(initialDelayString)) { + Assert.isTrue(initialDelay.isNegative(), "Specify 'initialDelay' or 'initialDelayString', not both"); + if (this.embeddedValueResolver != null) { + initialDelayString = this.embeddedValueResolver.resolveStringValue(initialDelayString); + } + if (StringUtils.hasLength(initialDelayString)) { + try { + initialDelay = toDuration(initialDelayString, scheduled.timeUnit()); + } + catch (RuntimeException ex) { + throw new IllegalArgumentException( + "Invalid initialDelayString value \"" + initialDelayString + "\" - cannot parse into long"); + } + } + } + + // Reject cron expression + Assert.state(!StringUtils.hasText(scheduled.cron()), "'cron' not supported for reactive @Scheduled"); + + // At this point we don't need to differentiate between initial delay set or not anymore + if (initialDelay.isNegative()) { + initialDelay = Duration.ZERO; + } + + // Check fixed delay + Duration fixedDelay = toDuration(scheduled.fixedDelay(), scheduled.timeUnit()); + if (!fixedDelay.isNegative()) { + processedSchedule = true; + reactiveTasks.add(new ScheduledAnnotationReactiveSupport.ReactiveTask(method, bean, initialDelay, fixedDelay, false)); + } + + String fixedDelayString = scheduled.fixedDelayString(); + if (StringUtils.hasText(fixedDelayString)) { + if (this.embeddedValueResolver != null) { + fixedDelayString = this.embeddedValueResolver.resolveStringValue(fixedDelayString); + } + if (StringUtils.hasLength(fixedDelayString)) { + Assert.isTrue(!processedSchedule, errorMessage); + processedSchedule = true; + try { + fixedDelay = toDuration(fixedDelayString, scheduled.timeUnit()); + } + catch (RuntimeException ex) { + throw new IllegalArgumentException( + "Invalid fixedDelayString value \"" + fixedDelayString + "\" - cannot parse into long"); + } + reactiveTasks.add(new ScheduledAnnotationReactiveSupport.ReactiveTask(method, bean, initialDelay, fixedDelay, false)); + } + } + + // Check fixed rate + Duration fixedRate = toDuration(scheduled.fixedRate(), scheduled.timeUnit()); + if (!fixedRate.isNegative()) { + Assert.isTrue(!processedSchedule, errorMessage); + processedSchedule = true; + reactiveTasks.add(new ScheduledAnnotationReactiveSupport.ReactiveTask(method, bean, initialDelay, fixedRate, true)); + } + String fixedRateString = scheduled.fixedRateString(); + if (StringUtils.hasText(fixedRateString)) { + if (this.embeddedValueResolver != null) { + fixedRateString = this.embeddedValueResolver.resolveStringValue(fixedRateString); + } + if (StringUtils.hasLength(fixedRateString)) { + Assert.isTrue(!processedSchedule, errorMessage); + processedSchedule = true; + try { + fixedRate = toDuration(fixedRateString, scheduled.timeUnit()); + } + catch (RuntimeException ex) { + throw new IllegalArgumentException( + "Invalid fixedRateString value \"" + fixedRateString + "\" - cannot parse into long"); + } + reactiveTasks.add(new ScheduledAnnotationReactiveSupport.ReactiveTask(method, bean, initialDelay, fixedRate, true)); + } + } + + // Check whether we had any attribute set + Assert.isTrue(processedSchedule, errorMessage); + + // Finally register the scheduled tasks (we synchronize on scheduledTasks on purpose) + synchronized (this.scheduledTasks) { + Set subscriptionTasks = this.scheduledReactiveTasks.computeIfAbsent(bean, key -> new LinkedHashSet<>(4)); + subscriptionTasks.addAll(reactiveTasks); + } + } + catch (IllegalArgumentException ex) { + throw new IllegalStateException( + "Encountered invalid reactive @Scheduled method '" + method.getName() + "': " + ex.getMessage(), ex); + } + } + /** * Create a {@link Runnable} for the given bean instance, * calling the specified scheduled method. @@ -554,6 +688,7 @@ private static boolean isP(char ch) { /** * Return all currently scheduled tasks, from {@link Scheduled} methods * as well as from programmatic {@link SchedulingConfigurer} interaction. + *

Note this doesn't include scheduled reactive methods. * @since 5.0.2 */ @Override @@ -572,20 +707,27 @@ public Set getScheduledTasks() { @Override public void postProcessBeforeDestruction(Object bean, String beanName) { Set tasks; + Set reactiveTasks; synchronized (this.scheduledTasks) { tasks = this.scheduledTasks.remove(bean); + reactiveTasks = this.scheduledReactiveTasks.remove(bean); } if (tasks != null) { for (ScheduledTask task : tasks) { task.cancel(); } } + if (reactiveTasks != null) { + for (ScheduledAnnotationReactiveSupport.ReactiveTask task : reactiveTasks) { + task.cancel(); + } + } } @Override public boolean requiresDestruction(Object bean) { synchronized (this.scheduledTasks) { - return this.scheduledTasks.containsKey(bean); + return this.scheduledTasks.containsKey(bean) || this.scheduledReactiveTasks.containsKey(bean); } } @@ -599,6 +741,13 @@ public void destroy() { } } this.scheduledTasks.clear(); + Collection> allReactiveTasks = this.scheduledReactiveTasks.values(); + for (Set tasks : allReactiveTasks) { + for (ScheduledAnnotationReactiveSupport.ReactiveTask task : tasks) { + task.cancel(); + } + } + this.scheduledReactiveTasks.clear(); } this.registrar.destroy(); } diff --git a/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupport.java b/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupport.java new file mode 100644 index 000000000000..a6a817d4d881 --- /dev/null +++ b/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupport.java @@ -0,0 +1,140 @@ +/* + * Copyright 2002-2023 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.scheduling.annotation; + +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; +import java.time.Duration; + +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; +import org.reactivestreams.Publisher; +import reactor.core.Disposable; +import reactor.core.Disposables; +import reactor.core.publisher.Flux; +import reactor.core.publisher.Mono; + +import org.springframework.aop.support.AopUtils; +import org.springframework.util.Assert; +import org.springframework.util.ClassUtils; +import org.springframework.util.ReflectionUtils; + +/** + * Helper class for @{@link ScheduledAnnotationBeanPostProcessor} to support reactive cases + * without a dependency on optional classes. + * @author Simon Baslé + * @since 6.0.x //FIXME + */ +abstract class ScheduledAnnotationReactiveSupport { + + static final boolean reactorPresent = ClassUtils.isPresent( + "reactor.core.publisher.Flux", ScheduledAnnotationReactiveSupport.class.getClassLoader()); + + static boolean isReactive(Method method) { + return Publisher.class.isAssignableFrom(method.getReturnType()); + } + + /** + * Encapsulates the logic of {@code @Scheduled} on reactive types, using Reactor. + */ + static class ReactiveTask { + + private final Publisher publisher; + private final Duration initialDelay; + private final Duration otherDelay; + private final boolean isFixedRate; + private final String checkpoint; + private final Disposable.Swap disposable; + + private final Log logger = LogFactory.getLog(getClass()); + + + protected ReactiveTask(Method method, Object bean, Duration initialDelay, + Duration otherDelay, boolean isFixedRate) { + + Assert.isTrue(method.getParameterCount() == 0, "Only no-arg methods may be annotated with @Scheduled"); + Method invocableMethod = AopUtils.selectInvocableMethod(method, bean.getClass()); + try { + ReflectionUtils.makeAccessible(invocableMethod); + Object r = invocableMethod.invoke(bean); + this.publisher = (Publisher) r; + } + catch (InvocationTargetException ex) { + throw new IllegalArgumentException("Cannot obtain a Publisher from the @Scheduled reactive method", ex.getTargetException()); + } + catch (IllegalAccessException ex) { + throw new IllegalArgumentException("Cannot obtain a Publisher from the @Scheduled reactive method", ex); + } + + this.initialDelay = initialDelay; + this.otherDelay = otherDelay; + this.isFixedRate = isFixedRate; + + this.disposable = Disposables.swap(); + this.checkpoint = "@Scheduled '"+ method.getDeclaringClass().getName() + + "#" + method.getName() + "()' [ScheduledAnnotationReactiveSupport]"; + } + + private Mono safeExecutionMono() { + Mono executionMono; + if (this.publisher instanceof Mono) { + executionMono = Mono.from(this.publisher).then(); + } + else { + executionMono = Flux.from(this.publisher).then(); + } + if (this.logger.isWarnEnabled()) { + executionMono = executionMono.doOnError(ex -> this.logger.warn( + "Ignored error in publisher from " + this.checkpoint, ex)); + } + executionMono = executionMono.onErrorComplete(); + return executionMono; + } + + public void subscribe() { + if (this.disposable.isDisposed()) { + return; + } + + final Mono executionMono = safeExecutionMono(); + Flux scheduledFlux; + if (this.isFixedRate) { + scheduledFlux = Flux.interval(this.initialDelay, this.otherDelay) + .flatMap(it -> executionMono); + } + else { + scheduledFlux = Mono.delay(this.otherDelay).then(executionMono).repeat(); + if (!this.initialDelay.isZero()) { + scheduledFlux = Flux.concat( + Mono.delay(this.initialDelay).then(executionMono), + scheduledFlux + ); + } + } + // Subscribe and ensure that errors can be traced back to the @Scheduled via a checkpoint + if (this.disposable.isDisposed()) { + return; + } + this.disposable.update(scheduledFlux.checkpoint(this.checkpoint) + .subscribe(it -> {}, ex -> ReactiveTask.this.logger.error("Unexpected error occurred in scheduled reactive task", ex))); + } + + public void cancel() { + this.disposable.dispose(); + } + } +} From f5b03c5522f3f36857fb822dcbe5b0429323ba58 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Basl=C3=A9?= Date: Fri, 3 Feb 2023 16:56:41 +0100 Subject: [PATCH 02/19] Add tests + fix initialDelay.ZERO case --- .../ScheduledAnnotationReactiveSupport.java | 13 +- ...heduledAnnotationReactiveSupportTests.java | 248 ++++++++++++++++++ 2 files changed, 259 insertions(+), 2 deletions(-) create mode 100644 spring-context/src/test/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupportTests.java diff --git a/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupport.java b/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupport.java index a6a817d4d881..aaab22cca4ca 100644 --- a/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupport.java +++ b/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupport.java @@ -89,6 +89,7 @@ protected ReactiveTask(Method method, Object bean, Duration initialDelay, + "#" + method.getName() + "()' [ScheduledAnnotationReactiveSupport]"; } + private Mono safeExecutionMono() { Mono executionMono; if (this.publisher instanceof Mono) { @@ -97,8 +98,8 @@ private Mono safeExecutionMono() { else { executionMono = Flux.from(this.publisher).then(); } - if (this.logger.isWarnEnabled()) { - executionMono = executionMono.doOnError(ex -> this.logger.warn( + if (logger.isWarnEnabled()) { + executionMono = executionMono.doOnError(ex -> logger.warn( "Ignored error in publisher from " + this.checkpoint, ex)); } executionMono = executionMono.onErrorComplete(); @@ -124,6 +125,9 @@ public void subscribe() { scheduledFlux ); } + else { + scheduledFlux = Flux.concat(executionMono, scheduledFlux); + } } // Subscribe and ensure that errors can be traced back to the @Scheduled via a checkpoint if (this.disposable.isDisposed()) { @@ -136,5 +140,10 @@ public void subscribe() { public void cancel() { this.disposable.dispose(); } + + @Override + public String toString() { + return this.checkpoint; + } } } diff --git a/spring-context/src/test/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupportTests.java b/spring-context/src/test/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupportTests.java new file mode 100644 index 000000000000..dcd1cb9627dc --- /dev/null +++ b/spring-context/src/test/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupportTests.java @@ -0,0 +1,248 @@ +/* + * Copyright 2002-2023 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.scheduling.annotation; + +import java.lang.reflect.Method; +import java.time.Duration; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.atomic.AtomicInteger; + +import org.awaitility.Awaitility; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; +import org.reactivestreams.Publisher; +import reactor.core.publisher.Flux; +import reactor.core.publisher.Mono; + +import org.springframework.util.ReflectionUtils; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; + +class ScheduledAnnotationReactiveSupportTests { + + @Test + void ensureReactor() { + assertThat(ScheduledAnnotationReactiveSupport.reactorPresent).isTrue(); + } + + @ParameterizedTest + @ValueSource(strings = { "mono", "flux", "monoString", "fluxString", "publisherMono", + "publisherString", "monoThrows" }) //note: monoWithParams can't be found by this test + void checkIsReactive(String method) { + Method m = ReflectionUtils.findMethod(ReactiveMethods.class, method); + assertThat(ScheduledAnnotationReactiveSupport.isReactive(m)).as(m.getName()).isTrue(); + } + + @Test + void checkNotReactive() { + Method string = ReflectionUtils.findMethod(ReactiveMethods.class, "oops"); + Method future = ReflectionUtils.findMethod(ReactiveMethods.class, "future"); + + assertThat(ScheduledAnnotationReactiveSupport.isReactive(string)) + .as("String-returning").isFalse(); + assertThat(ScheduledAnnotationReactiveSupport.isReactive(future)) + .as("Future-returning").isFalse(); + } + + static class ReactiveMethods { + + public String oops() { + return "oops"; + } + + public Mono mono() { + return Mono.empty(); + } + + public Flux flux() { + return Flux.empty(); + } + + public Mono monoString() { + return Mono.just("example"); + } + + public Flux fluxString() { + return Flux.just("example"); + } + + public Publisher publisherMono() { + return Mono.empty(); + } + + public Publisher publisherString() { + return fluxString(); + } + + public CompletableFuture future() { + return CompletableFuture.completedFuture("example"); + } + + public Mono monoWithParam(String param) { + return Mono.just(param).then(); + } + + public Mono monoThrows() { + throw new IllegalStateException("expected"); + } + + public Mono monoThrowsIllegalAccess() throws IllegalAccessException { + //simulate a reflection issue + throw new IllegalAccessException("expected"); + } + + AtomicInteger subscription = new AtomicInteger(); + + public Mono trackingMono() { + return Mono.empty() + .doOnSubscribe(s -> subscription.incrementAndGet()); + } + + public Mono monoError() { + return Mono.error(new IllegalStateException("expected")); + } + + } + + @Nested + class ReactiveTaskTests { + + private ReactiveMethods target; + + @BeforeEach + void init() { + this.target = new ReactiveMethods(); + } + + @Test + void rejectWithParams() { + Method m = ReflectionUtils.findMethod(ReactiveMethods.class, "monoWithParam", String.class); + + assertThat(ScheduledAnnotationReactiveSupport.isReactive(m)).as("isReactive").isTrue(); + + assertThatIllegalArgumentException().isThrownBy(() -> new ScheduledAnnotationReactiveSupport.ReactiveTask( + m, target, Duration.ZERO, Duration.ZERO, false)) + .withMessage("Only no-arg methods may be annotated with @Scheduled") + .withNoCause(); + } + + @Test + void rejectCantProducePublisher() { + Method m = ReflectionUtils.findMethod(ReactiveMethods.class, "monoThrows"); + + assertThatIllegalArgumentException().isThrownBy(() -> new ScheduledAnnotationReactiveSupport.ReactiveTask( + m, target, Duration.ZERO, Duration.ZERO, false)) + .withMessage("Cannot obtain a Publisher from the @Scheduled reactive method") + .withCause(new IllegalStateException("expected")); + } + + @Test + void rejectCantAccessMethod() { + Method m = ReflectionUtils.findMethod(ReactiveMethods.class, "monoThrowsIllegalAccess"); + + assertThatIllegalArgumentException().isThrownBy(() -> new ScheduledAnnotationReactiveSupport.ReactiveTask( + m, target, Duration.ZERO, Duration.ZERO, false)) + .withMessage("Cannot obtain a Publisher from the @Scheduled reactive method") + .withCause(new IllegalAccessException("expected")); + } + + @Test + void hasCheckpointToString() { + Method m = ReflectionUtils.findMethod(ReactiveMethods.class, "mono"); + final ScheduledAnnotationReactiveSupport.ReactiveTask reactiveTask = new ScheduledAnnotationReactiveSupport.ReactiveTask( + m, target, Duration.ZERO, Duration.ZERO, false); + + assertThat(reactiveTask).hasToString("@Scheduled 'org.springframework.scheduling.annotation.ScheduledAnnotationReactiveSupportTests$ReactiveMethods#mono()' [ScheduledAnnotationReactiveSupport]"); + } + + @Test + void cancelledEarlyPreventsSubscription() { + Method m = ReflectionUtils.findMethod(ReactiveMethods.class, "trackingMono"); + final ScheduledAnnotationReactiveSupport.ReactiveTask reactiveTask = new ScheduledAnnotationReactiveSupport.ReactiveTask( + m, target, Duration.ZERO, Duration.ofSeconds(10), false); + reactiveTask.cancel(); + reactiveTask.subscribe(); + + assertThat(target.subscription).hasValue(0); + } + + @Test + void noInitialDelayFixedDelay() throws InterruptedException { + Method m = ReflectionUtils.findMethod(target.getClass(), "trackingMono"); + final ScheduledAnnotationReactiveSupport.ReactiveTask reactiveTask = new ScheduledAnnotationReactiveSupport.ReactiveTask( + m, target, Duration.ZERO, Duration.ofSeconds(10), false); + reactiveTask.subscribe(); + Thread.sleep(500); + reactiveTask.cancel(); + + assertThat(target.subscription).hasValue(1); + } + + @Test + void noInitialDelayFixedRate() throws InterruptedException { + Method m = ReflectionUtils.findMethod(target.getClass(), "trackingMono"); + final ScheduledAnnotationReactiveSupport.ReactiveTask reactiveTask = new ScheduledAnnotationReactiveSupport.ReactiveTask( + m, target, Duration.ZERO, Duration.ofSeconds(10), true); + reactiveTask.subscribe(); + Thread.sleep(500); + reactiveTask.cancel(); + + assertThat(target.subscription).hasValue(1); + } + + @Test + void initialDelayFixedDelay() throws InterruptedException { + Method m = ReflectionUtils.findMethod(target.getClass(), "trackingMono"); + final ScheduledAnnotationReactiveSupport.ReactiveTask reactiveTask = new ScheduledAnnotationReactiveSupport.ReactiveTask( + m, target, Duration.ofSeconds(10), Duration.ofMillis(500), false); + reactiveTask.subscribe(); + Thread.sleep(500); + reactiveTask.cancel(); + + assertThat(target.subscription).hasValue(0); + } + + @Test + void initialDelayFixedRate() throws InterruptedException { + Method m = ReflectionUtils.findMethod(target.getClass(), "trackingMono"); + final ScheduledAnnotationReactiveSupport.ReactiveTask reactiveTask = new ScheduledAnnotationReactiveSupport.ReactiveTask( + m, target, Duration.ofSeconds(10), Duration.ofMillis(500), true); + reactiveTask.subscribe(); + Thread.sleep(500); + reactiveTask.cancel(); + + assertThat(target.subscription).hasValue(0); + } + + @Test + void monoErrorHasCheckpoint() throws InterruptedException { + Method m = ReflectionUtils.findMethod(target.getClass(), "monoError"); + final ScheduledAnnotationReactiveSupport.ReactiveTask reactiveTask = new ScheduledAnnotationReactiveSupport.ReactiveTask( + m, target, Duration.ZERO, Duration.ofSeconds(10), true); + + reactiveTask.subscribe(); + Thread.sleep(500); + + reactiveTask.cancel(); + } + + } +} \ No newline at end of file From 3c11661be864320f37ed40e27935e4d9fee3d897 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Basl=C3=A9?= Date: Mon, 6 Feb 2023 10:17:10 +0100 Subject: [PATCH 03/19] fix checkpoint test and checkstyle polish --- .../annotation/ScheduledAnnotationReactiveSupport.java | 2 +- .../ScheduledAnnotationReactiveSupportTests.java | 10 ++++------ 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupport.java b/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupport.java index aaab22cca4ca..7d7fac096399 100644 --- a/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupport.java +++ b/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupport.java @@ -57,7 +57,7 @@ static class ReactiveTask { private final Duration initialDelay; private final Duration otherDelay; private final boolean isFixedRate; - private final String checkpoint; + protected final String checkpoint; private final Disposable.Swap disposable; private final Log logger = LogFactory.getLog(getClass()); diff --git a/spring-context/src/test/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupportTests.java b/spring-context/src/test/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupportTests.java index dcd1cb9627dc..c4a09be1a359 100644 --- a/spring-context/src/test/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupportTests.java +++ b/spring-context/src/test/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupportTests.java @@ -21,7 +21,6 @@ import java.util.concurrent.CompletableFuture; import java.util.concurrent.atomic.AtomicInteger; -import org.awaitility.Awaitility; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; @@ -238,11 +237,10 @@ void monoErrorHasCheckpoint() throws InterruptedException { final ScheduledAnnotationReactiveSupport.ReactiveTask reactiveTask = new ScheduledAnnotationReactiveSupport.ReactiveTask( m, target, Duration.ZERO, Duration.ofSeconds(10), true); - reactiveTask.subscribe(); - Thread.sleep(500); - - reactiveTask.cancel(); + assertThat(reactiveTask.checkpoint).isEqualTo("@Scheduled 'org.springframework.scheduling.annotation" + + ".ScheduledAnnotationReactiveSupportTests$ReactiveMethods#monoError()'" + + " [ScheduledAnnotationReactiveSupport]"); } } -} \ No newline at end of file +} From 139f9467fdc46abf7af212ab7c9e56a6765dec11 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Basl=C3=A9?= Date: Tue, 14 Feb 2023 16:23:58 +0100 Subject: [PATCH 04/19] Add support for Kotlin suspending functions Closes gh-28515 --- spring-context/spring-context.gradle | 2 +- .../ScheduledAnnotationBeanPostProcessor.java | 37 +-- .../ScheduledAnnotationReactiveSupport.java | 100 ++++++-- ...heduledAnnotationReactiveSupportTests.java | 50 ++-- ...ScheduledAnnotationReactiveSupportTests.kt | 219 ++++++++++++++++++ 5 files changed, 360 insertions(+), 48 deletions(-) create mode 100644 spring-context/src/test/kotlin/org/springframework/scheduling/annotation/KotlinScheduledAnnotationReactiveSupportTests.kt diff --git a/spring-context/spring-context.gradle b/spring-context/spring-context.gradle index 86e14c454dd2..2fcf0d027579 100644 --- a/spring-context/spring-context.gradle +++ b/spring-context/spring-context.gradle @@ -38,7 +38,7 @@ dependencies { testImplementation("org.apache.commons:commons-pool2") testImplementation("org.awaitility:awaitility") testImplementation("jakarta.inject:jakarta.inject-tck") - testImplementation("org.jetbrains.kotlinx:kotlinx-coroutines-core") + testImplementation("org.jetbrains.kotlinx:kotlinx-coroutines-reactor") testRuntimeOnly("jakarta.xml.bind:jakarta.xml.bind-api") testRuntimeOnly("org.glassfish:jakarta.el") // Substitute for javax.management:jmxremote_optional:1.0.1_04 (not available on Maven Central) diff --git a/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationBeanPostProcessor.java b/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationBeanPostProcessor.java index cff077325a25..8cd127a39927 100644 --- a/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationBeanPostProcessor.java +++ b/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationBeanPostProcessor.java @@ -396,12 +396,15 @@ public Object postProcessAfterInitialization(Object bean, String beanName) { } protected void processScheduled(Scheduled scheduled, Method method, Object bean) { - // If Reactor is on the classpath, check for methods that return a Publisher - if (ScheduledAnnotationReactiveSupport.reactorPresent) { - if (ScheduledAnnotationReactiveSupport.isReactive(method)) { - processScheduledReactive(scheduled, method, bean); - return; - } + // Check for Kotlin suspending functions. If one is found but reactor bridge isn't on the classpath, throw + if (ScheduledAnnotationReactiveSupport.checkKotlinRuntimeIfNeeded(method)) { + processScheduledReactive(scheduled, method, bean, true); + return; + } + // Check for Publisher-returning methods. If found but Reactor isn't on the classpath, throw. + if (ScheduledAnnotationReactiveSupport.checkReactorRuntimeIfNeeded(method)) { + processScheduledReactive(scheduled, method, bean, false); + return; } processScheduledSync(scheduled, method, bean); } @@ -539,16 +542,18 @@ protected void processScheduledSync(Scheduled scheduled, Method method, Object b } /** - * Process the given {@code @Scheduled} method declaration on the given bean which - * returns a {@code Publisher}, repeatedly subscribing to the returned publisher - * according to the fixedDelay/fixedRate configuration. Cron configuration isn't supported, - * nor is non-Publisher return types (even if a {@code ReactiveAdapter} is registered). + * Process the given {@code @Scheduled} bean method declaration which returns + * a {@code Publisher}, or the given Kotlin suspending function converted to a + * Publisher. The publisher is then repeatedly subscribed to, according to the + * fixedDelay/fixedRate configuration. Cron configuration isn't supported,nor + * is non-Publisher return types (even if a {@code ReactiveAdapter} is registered). * @param scheduled the {@code @Scheduled} annotation - * @param method the method that the annotation has been declared on, which MUST return a Publisher + * @param method the method that the annotation has been declared on, which + * MUST either return a Publisher or be a Kotlin suspending function * @param bean the target bean instance * @see #createRunnable(Object, Method) */ - protected void processScheduledReactive(Scheduled scheduled, Method method, Object bean) { + protected void processScheduledReactive(Scheduled scheduled, Method method, Object bean, boolean isSuspendingFunction) { try { boolean processedSchedule = false; String errorMessage = @@ -587,7 +592,7 @@ protected void processScheduledReactive(Scheduled scheduled, Method method, Obje Duration fixedDelay = toDuration(scheduled.fixedDelay(), scheduled.timeUnit()); if (!fixedDelay.isNegative()) { processedSchedule = true; - reactiveTasks.add(new ScheduledAnnotationReactiveSupport.ReactiveTask(method, bean, initialDelay, fixedDelay, false)); + reactiveTasks.add(new ScheduledAnnotationReactiveSupport.ReactiveTask(method, bean, initialDelay, fixedDelay, false, isSuspendingFunction)); } String fixedDelayString = scheduled.fixedDelayString(); @@ -605,7 +610,7 @@ protected void processScheduledReactive(Scheduled scheduled, Method method, Obje throw new IllegalArgumentException( "Invalid fixedDelayString value \"" + fixedDelayString + "\" - cannot parse into long"); } - reactiveTasks.add(new ScheduledAnnotationReactiveSupport.ReactiveTask(method, bean, initialDelay, fixedDelay, false)); + reactiveTasks.add(new ScheduledAnnotationReactiveSupport.ReactiveTask(method, bean, initialDelay, fixedDelay, false, isSuspendingFunction)); } } @@ -614,7 +619,7 @@ protected void processScheduledReactive(Scheduled scheduled, Method method, Obje if (!fixedRate.isNegative()) { Assert.isTrue(!processedSchedule, errorMessage); processedSchedule = true; - reactiveTasks.add(new ScheduledAnnotationReactiveSupport.ReactiveTask(method, bean, initialDelay, fixedRate, true)); + reactiveTasks.add(new ScheduledAnnotationReactiveSupport.ReactiveTask(method, bean, initialDelay, fixedRate, true, isSuspendingFunction)); } String fixedRateString = scheduled.fixedRateString(); if (StringUtils.hasText(fixedRateString)) { @@ -631,7 +636,7 @@ protected void processScheduledReactive(Scheduled scheduled, Method method, Obje throw new IllegalArgumentException( "Invalid fixedRateString value \"" + fixedRateString + "\" - cannot parse into long"); } - reactiveTasks.add(new ScheduledAnnotationReactiveSupport.ReactiveTask(method, bean, initialDelay, fixedRate, true)); + reactiveTasks.add(new ScheduledAnnotationReactiveSupport.ReactiveTask(method, bean, initialDelay, fixedRate, true, isSuspendingFunction)); } } diff --git a/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupport.java b/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupport.java index 7d7fac096399..73c64682f1f3 100644 --- a/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupport.java +++ b/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupport.java @@ -29,6 +29,8 @@ import reactor.core.publisher.Mono; import org.springframework.aop.support.AopUtils; +import org.springframework.core.CoroutinesUtils; +import org.springframework.core.KotlinDetector; import org.springframework.util.Assert; import org.springframework.util.ClassUtils; import org.springframework.util.ReflectionUtils; @@ -41,11 +43,86 @@ */ abstract class ScheduledAnnotationReactiveSupport { + static final boolean publisherPresent = ClassUtils.isPresent( + "org.reactivestreams.Publisher", ScheduledAnnotationReactiveSupport.class.getClassLoader()); + static final boolean reactorPresent = ClassUtils.isPresent( "reactor.core.publisher.Flux", ScheduledAnnotationReactiveSupport.class.getClassLoader()); - static boolean isReactive(Method method) { - return Publisher.class.isAssignableFrom(method.getReturnType()); + static final boolean coroutinesReactorPresent = ClassUtils.isPresent( + "kotlinx.coroutines.reactor.MonoKt", ScheduledAnnotationReactiveSupport.class.getClassLoader()); + + /** + * Checks that if the method is reactive, it can be scheduled. If it isn't reactive + * (Reactive Streams {@code Publisher} is not present at runtime or the method doesn't + * return a form of Publisher) then this check returns {@code false}. Otherwise, it is + * eligible for reactive scheduling and Reactor MUST also be present at runtime. + * Provided that is the case, this method returns {@code true}. Otherwise, it throws + * an {@code IllegalStateException}. + */ + static boolean checkReactorRuntimeIfNeeded(Method method) { + if (!(publisherPresent && Publisher.class.isAssignableFrom(method.getReturnType()))) { + return false; + } + if (reactorPresent) { + return true; + } + throw new IllegalStateException("Reactive methods returning a Publisher may only be annotated with @Scheduled" + + "if Reactor is present at runtime"); + } + + /** + * Checks that if the method is a Kotlin suspending function, it can be scheduled. + * If it isn't a suspending function (or Kotlin is not detected at runtime) then this + * check returns {@code false}. Otherwise, it is eligible for conversion to a + * {@code Mono} for reactive scheduling and both Reactor and the + * {@code kotlinx.coroutines.reactor} bridge MUST also be present at runtime. + * Provided that is the case, this method returns {@code true}. Otherwise, it throws + * an {@code IllegalStateException}. + */ + static boolean checkKotlinRuntimeIfNeeded(Method method) { + if (!(KotlinDetector.isKotlinPresent() && KotlinDetector.isSuspendingFunction(method))) { + return false; + } + if (coroutinesReactorPresent) { + return true; + } + throw new IllegalStateException("Kotlin suspending functions may only be annotated with @Scheduled" + + "if Reactor and the Reactor-Coroutine bridge (kotlinx.coroutines.reactor) are present at runtime"); + } + + /** + * Reflectively invoke a reactive method and return the resulting {@code Publisher}. + * The {@link #checkReactorRuntimeIfNeeded(Method)} check is a precondition to calling + * this method. + */ + static Publisher getPublisherForReactiveMethod(Method method, Object bean) { + Assert.isTrue(method.getParameterCount() == 0, "Only no-arg reactive methods may be annotated with @Scheduled"); + Method invocableMethod = AopUtils.selectInvocableMethod(method, bean.getClass()); + try { + ReflectionUtils.makeAccessible(invocableMethod); + Object r = invocableMethod.invoke(bean); + return (Publisher) r; + } + catch (InvocationTargetException ex) { + throw new IllegalArgumentException("Cannot obtain a Publisher from the @Scheduled reactive method", ex.getTargetException()); + } + catch (IllegalAccessException ex) { + throw new IllegalArgumentException("Cannot obtain a Publisher from the @Scheduled reactive method", ex); + } + } + + /** + * Turn the provided suspending function method into a {@code Publisher} via + * {@link CoroutinesUtils} and return that Publisher. + * The {@link #checkKotlinRuntimeIfNeeded(Method)} check is a precondition to calling + * this method. + */ + static Publisher getPublisherForSuspendingFunction(Method method, Object bean) { + Assert.isTrue(method.getParameterCount() == 1,"Only no-args suspending functions may be " + + "annotated with @Scheduled (with 1 self-referencing synthetic arg expected)"); + + return CoroutinesUtils.invokeSuspendingFunction(method, bean, (Object[]) method.getParameters()); } /** @@ -64,20 +141,12 @@ static class ReactiveTask { protected ReactiveTask(Method method, Object bean, Duration initialDelay, - Duration otherDelay, boolean isFixedRate) { - - Assert.isTrue(method.getParameterCount() == 0, "Only no-arg methods may be annotated with @Scheduled"); - Method invocableMethod = AopUtils.selectInvocableMethod(method, bean.getClass()); - try { - ReflectionUtils.makeAccessible(invocableMethod); - Object r = invocableMethod.invoke(bean); - this.publisher = (Publisher) r; + Duration otherDelay, boolean isFixedRate, boolean isSuspendingFunction) { + if (isSuspendingFunction) { + this.publisher = getPublisherForSuspendingFunction(method, bean); } - catch (InvocationTargetException ex) { - throw new IllegalArgumentException("Cannot obtain a Publisher from the @Scheduled reactive method", ex.getTargetException()); - } - catch (IllegalAccessException ex) { - throw new IllegalArgumentException("Cannot obtain a Publisher from the @Scheduled reactive method", ex); + else { + this.publisher = getPublisherForReactiveMethod(method, bean); } this.initialDelay = initialDelay; @@ -89,7 +158,6 @@ protected ReactiveTask(Method method, Object bean, Duration initialDelay, + "#" + method.getName() + "()' [ScheduledAnnotationReactiveSupport]"; } - private Mono safeExecutionMono() { Mono executionMono; if (this.publisher instanceof Mono) { diff --git a/spring-context/src/test/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupportTests.java b/spring-context/src/test/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupportTests.java index c4a09be1a359..6b4a435c2dc5 100644 --- a/spring-context/src/test/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupportTests.java +++ b/spring-context/src/test/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupportTests.java @@ -34,6 +34,7 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; +import static org.springframework.scheduling.annotation.ScheduledAnnotationReactiveSupport.getPublisherForReactiveMethod; class ScheduledAnnotationReactiveSupportTests { @@ -47,7 +48,7 @@ void ensureReactor() { "publisherString", "monoThrows" }) //note: monoWithParams can't be found by this test void checkIsReactive(String method) { Method m = ReflectionUtils.findMethod(ReactiveMethods.class, method); - assertThat(ScheduledAnnotationReactiveSupport.isReactive(m)).as(m.getName()).isTrue(); + assertThat(ScheduledAnnotationReactiveSupport.checkReactorRuntimeIfNeeded(m)).as(m.getName()).isTrue(); } @Test @@ -55,9 +56,9 @@ void checkNotReactive() { Method string = ReflectionUtils.findMethod(ReactiveMethods.class, "oops"); Method future = ReflectionUtils.findMethod(ReactiveMethods.class, "future"); - assertThat(ScheduledAnnotationReactiveSupport.isReactive(string)) + assertThat(ScheduledAnnotationReactiveSupport.checkReactorRuntimeIfNeeded(string)) .as("String-returning").isFalse(); - assertThat(ScheduledAnnotationReactiveSupport.isReactive(future)) + assertThat(ScheduledAnnotationReactiveSupport.checkReactorRuntimeIfNeeded(future)) .as("Future-returning").isFalse(); } @@ -131,15 +132,22 @@ void init() { this.target = new ReactiveMethods(); } + @SuppressWarnings("ReactiveStreamsUnusedPublisher") @Test void rejectWithParams() { Method m = ReflectionUtils.findMethod(ReactiveMethods.class, "monoWithParam", String.class); - assertThat(ScheduledAnnotationReactiveSupport.isReactive(m)).as("isReactive").isTrue(); + assertThat(ScheduledAnnotationReactiveSupport.checkReactorRuntimeIfNeeded(m)).as("isReactive").isTrue(); + //static helper method + assertThatIllegalArgumentException().isThrownBy(() -> getPublisherForReactiveMethod(m, target)) + .withMessage("Only no-arg reactive methods may be annotated with @Scheduled") + .withNoCause(); + + //constructor of task assertThatIllegalArgumentException().isThrownBy(() -> new ScheduledAnnotationReactiveSupport.ReactiveTask( - m, target, Duration.ZERO, Duration.ZERO, false)) - .withMessage("Only no-arg methods may be annotated with @Scheduled") + m, target, Duration.ZERO, Duration.ZERO, false, false)) + .withMessage("Only no-arg reactive methods may be annotated with @Scheduled") .withNoCause(); } @@ -147,8 +155,14 @@ void rejectWithParams() { void rejectCantProducePublisher() { Method m = ReflectionUtils.findMethod(ReactiveMethods.class, "monoThrows"); + //static helper method + assertThatIllegalArgumentException().isThrownBy(() -> getPublisherForReactiveMethod(m, target)) + .withMessage("Cannot obtain a Publisher from the @Scheduled reactive method") + .withCause(new IllegalStateException("expected")); + + //constructor of task assertThatIllegalArgumentException().isThrownBy(() -> new ScheduledAnnotationReactiveSupport.ReactiveTask( - m, target, Duration.ZERO, Duration.ZERO, false)) + m, target, Duration.ZERO, Duration.ZERO, false, false)) .withMessage("Cannot obtain a Publisher from the @Scheduled reactive method") .withCause(new IllegalStateException("expected")); } @@ -157,8 +171,14 @@ void rejectCantProducePublisher() { void rejectCantAccessMethod() { Method m = ReflectionUtils.findMethod(ReactiveMethods.class, "monoThrowsIllegalAccess"); + //static helper method + assertThatIllegalArgumentException().isThrownBy(() -> getPublisherForReactiveMethod(m, target)) + .withMessage("Cannot obtain a Publisher from the @Scheduled reactive method") + .withCause(new IllegalAccessException("expected")); + + //constructor of task assertThatIllegalArgumentException().isThrownBy(() -> new ScheduledAnnotationReactiveSupport.ReactiveTask( - m, target, Duration.ZERO, Duration.ZERO, false)) + m, target, Duration.ZERO, Duration.ZERO, false, false)) .withMessage("Cannot obtain a Publisher from the @Scheduled reactive method") .withCause(new IllegalAccessException("expected")); } @@ -167,7 +187,7 @@ void rejectCantAccessMethod() { void hasCheckpointToString() { Method m = ReflectionUtils.findMethod(ReactiveMethods.class, "mono"); final ScheduledAnnotationReactiveSupport.ReactiveTask reactiveTask = new ScheduledAnnotationReactiveSupport.ReactiveTask( - m, target, Duration.ZERO, Duration.ZERO, false); + m, target, Duration.ZERO, Duration.ZERO, false, false); assertThat(reactiveTask).hasToString("@Scheduled 'org.springframework.scheduling.annotation.ScheduledAnnotationReactiveSupportTests$ReactiveMethods#mono()' [ScheduledAnnotationReactiveSupport]"); } @@ -176,7 +196,7 @@ void hasCheckpointToString() { void cancelledEarlyPreventsSubscription() { Method m = ReflectionUtils.findMethod(ReactiveMethods.class, "trackingMono"); final ScheduledAnnotationReactiveSupport.ReactiveTask reactiveTask = new ScheduledAnnotationReactiveSupport.ReactiveTask( - m, target, Duration.ZERO, Duration.ofSeconds(10), false); + m, target, Duration.ZERO, Duration.ofSeconds(10), false, false); reactiveTask.cancel(); reactiveTask.subscribe(); @@ -187,7 +207,7 @@ void cancelledEarlyPreventsSubscription() { void noInitialDelayFixedDelay() throws InterruptedException { Method m = ReflectionUtils.findMethod(target.getClass(), "trackingMono"); final ScheduledAnnotationReactiveSupport.ReactiveTask reactiveTask = new ScheduledAnnotationReactiveSupport.ReactiveTask( - m, target, Duration.ZERO, Duration.ofSeconds(10), false); + m, target, Duration.ZERO, Duration.ofSeconds(10), false, false); reactiveTask.subscribe(); Thread.sleep(500); reactiveTask.cancel(); @@ -199,7 +219,7 @@ void noInitialDelayFixedDelay() throws InterruptedException { void noInitialDelayFixedRate() throws InterruptedException { Method m = ReflectionUtils.findMethod(target.getClass(), "trackingMono"); final ScheduledAnnotationReactiveSupport.ReactiveTask reactiveTask = new ScheduledAnnotationReactiveSupport.ReactiveTask( - m, target, Duration.ZERO, Duration.ofSeconds(10), true); + m, target, Duration.ZERO, Duration.ofSeconds(10), true, false); reactiveTask.subscribe(); Thread.sleep(500); reactiveTask.cancel(); @@ -211,7 +231,7 @@ void noInitialDelayFixedRate() throws InterruptedException { void initialDelayFixedDelay() throws InterruptedException { Method m = ReflectionUtils.findMethod(target.getClass(), "trackingMono"); final ScheduledAnnotationReactiveSupport.ReactiveTask reactiveTask = new ScheduledAnnotationReactiveSupport.ReactiveTask( - m, target, Duration.ofSeconds(10), Duration.ofMillis(500), false); + m, target, Duration.ofSeconds(10), Duration.ofMillis(500), false, false); reactiveTask.subscribe(); Thread.sleep(500); reactiveTask.cancel(); @@ -223,7 +243,7 @@ void initialDelayFixedDelay() throws InterruptedException { void initialDelayFixedRate() throws InterruptedException { Method m = ReflectionUtils.findMethod(target.getClass(), "trackingMono"); final ScheduledAnnotationReactiveSupport.ReactiveTask reactiveTask = new ScheduledAnnotationReactiveSupport.ReactiveTask( - m, target, Duration.ofSeconds(10), Duration.ofMillis(500), true); + m, target, Duration.ofSeconds(10), Duration.ofMillis(500), true, false); reactiveTask.subscribe(); Thread.sleep(500); reactiveTask.cancel(); @@ -235,7 +255,7 @@ void initialDelayFixedRate() throws InterruptedException { void monoErrorHasCheckpoint() throws InterruptedException { Method m = ReflectionUtils.findMethod(target.getClass(), "monoError"); final ScheduledAnnotationReactiveSupport.ReactiveTask reactiveTask = new ScheduledAnnotationReactiveSupport.ReactiveTask( - m, target, Duration.ZERO, Duration.ofSeconds(10), true); + m, target, Duration.ZERO, Duration.ofSeconds(10), true, false); assertThat(reactiveTask.checkpoint).isEqualTo("@Scheduled 'org.springframework.scheduling.annotation" + ".ScheduledAnnotationReactiveSupportTests$ReactiveMethods#monoError()'" diff --git a/spring-context/src/test/kotlin/org/springframework/scheduling/annotation/KotlinScheduledAnnotationReactiveSupportTests.kt b/spring-context/src/test/kotlin/org/springframework/scheduling/annotation/KotlinScheduledAnnotationReactiveSupportTests.kt new file mode 100644 index 000000000000..deae03db5e23 --- /dev/null +++ b/spring-context/src/test/kotlin/org/springframework/scheduling/annotation/KotlinScheduledAnnotationReactiveSupportTests.kt @@ -0,0 +1,219 @@ +/* + * Copyright 2002-2023 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.scheduling.annotation + +import org.assertj.core.api.Assertions +import org.assertj.core.api.Assertions.assertThat +import org.junit.jupiter.api.BeforeEach +import org.junit.jupiter.api.Test +import org.springframework.scheduling.annotation.ScheduledAnnotationReactiveSupport.ReactiveTask +import org.springframework.scheduling.annotation.ScheduledAnnotationReactiveSupport.checkKotlinRuntimeIfNeeded +import org.springframework.scheduling.annotation.ScheduledAnnotationReactiveSupport.getPublisherForSuspendingFunction +import org.springframework.scheduling.annotation.ScheduledAnnotationReactiveSupportTests.ReactiveMethods +import org.springframework.util.ReflectionUtils +import reactor.core.publisher.Mono +import java.time.Duration +import java.util.concurrent.atomic.AtomicInteger +import kotlin.coroutines.Continuation + +class KotlinScheduledAnnotationReactiveSupportTests { + + @Test + fun ensureReactor() { + Assertions.assertThat(ScheduledAnnotationReactiveSupport.reactorPresent).isTrue + } + + @Test + fun ensureKotlinCoroutineReactorBridge() { + Assertions.assertThat(ScheduledAnnotationReactiveSupport.coroutinesReactorPresent).isTrue + } + + @Test + fun kotlinSuspendingFunctionIsNotReactive() { + val suspending = ReflectionUtils.findMethod(SuspendingFunctions::class.java, "suspending", Continuation::class.java) + Assertions.assertThat(ScheduledAnnotationReactiveSupport.checkReactorRuntimeIfNeeded(suspending!!)).isFalse + } + + internal class SuspendingFunctions { + suspend fun suspending() { + } + + suspend fun suspendingReturns(): String = "suspended" + + suspend fun withParam(param: String): String { + return param + } + + suspend fun throwsIllegalState() { + throw IllegalStateException("expected") + } + + var subscription = AtomicInteger() + suspend fun suspendingTracking() { + subscription.incrementAndGet() + } + + fun notSuspending() { } + } + + + private var target: SuspendingFunctions? = null + + @BeforeEach + fun init() { + target = SuspendingFunctions() + } + + @Test + fun checkKotlinRuntimeIfNeeded() { + val suspendingMethod = ReflectionUtils.findMethod(SuspendingFunctions::class.java, "suspending", Continuation::class.java)!! + val notSuspendingMethod = ReflectionUtils.findMethod(SuspendingFunctions::class.java, "notSuspending")!! + + assertThat(checkKotlinRuntimeIfNeeded(suspendingMethod)).describedAs("suspending").isTrue() + assertThat(checkKotlinRuntimeIfNeeded(notSuspendingMethod)).describedAs("not suspending").isFalse() + } + + @Test + fun rejectWithParams() { + val m = ReflectionUtils.findMethod(SuspendingFunctions::class.java, "withParam", String::class.java, Continuation::class.java) + + //static helper method + Assertions.assertThatIllegalArgumentException().isThrownBy { getPublisherForSuspendingFunction(m!!, target!!) } + .withMessage("Only no-args suspending functions may be annotated with @Scheduled (with 1 self-referencing synthetic arg expected)") + .withNoCause() + + //constructor of task + Assertions.assertThatIllegalArgumentException().isThrownBy { + ReactiveTask(m!!, target!!, Duration.ZERO, Duration.ZERO, false, true) + } + .withMessage("Only no-args suspending functions may be annotated with @Scheduled (with 1 self-referencing synthetic arg expected)") + .withNoCause() + } + + @Test + fun rejectNotSuspending() { + val m = ReflectionUtils.findMethod(SuspendingFunctions::class.java, "notSuspending") + + //static helper method + Assertions.assertThatIllegalArgumentException().isThrownBy { getPublisherForSuspendingFunction(m!!, target!!) } + .withMessage("Only no-args suspending functions may be annotated with @Scheduled (with 1 self-referencing synthetic arg expected)") + .withNoCause() + + //constructor of task + Assertions.assertThatIllegalArgumentException().isThrownBy { + ReactiveTask(m!!, target!!, Duration.ZERO, Duration.ZERO, false, true) + } + .withMessage("Only no-args suspending functions may be annotated with @Scheduled (with 1 self-referencing synthetic arg expected)") + .withNoCause() + } + + @Test + fun suspendingThrowIsTurnedToMonoError() { + val m = ReflectionUtils.findMethod(SuspendingFunctions::class.java, "throwsIllegalState", Continuation::class.java) + + val mono = Mono.from(getPublisherForSuspendingFunction(m!!, target!!)) + + Assertions.assertThatIllegalStateException().isThrownBy { mono.block() } + .withMessage("expected") + .withNoCause() + + //constructor of task doesn't throw + Assertions.assertThatNoException().isThrownBy { + ReactiveTask(m, target!!, Duration.ZERO, Duration.ZERO, false, true) + } + } + + @Test + fun turningSuspendingFunctionToMonoDoesntExecuteTheMethod() { + val m = ReflectionUtils.findMethod(SuspendingFunctions::class.java, "suspendingTracking", Continuation::class.java) + val mono = Mono.from(getPublisherForSuspendingFunction(m!!, target!!)) + + assertThat(target!!.subscription).hasValue(0) + mono.block() + assertThat(target!!.subscription).describedAs("after subscription").hasValue(1) + } + + @Test + fun hasCheckpointToString() { + val m = ReflectionUtils.findMethod(SuspendingFunctions::class.java, "suspending", Continuation::class.java) + val reactiveTask = ReactiveTask(m!!, target!!, Duration.ZERO, Duration.ZERO, false, true) + assertThat(reactiveTask).hasToString("@Scheduled 'org.springframework.scheduling.annotation.KotlinScheduledAnnotationReactiveSupportTests\$SuspendingFunctions#suspending()' [ScheduledAnnotationReactiveSupport]") + } + + @Test + fun cancelledEarlyPreventsSubscription() { + val m = ReflectionUtils.findMethod(SuspendingFunctions::class.java, "suspendingTracking", Continuation::class.java) + val reactiveTask = ReactiveTask(m!!, target!!, Duration.ZERO, Duration.ofSeconds(10), false, true) + reactiveTask.cancel() + reactiveTask.subscribe() + assertThat(target!!.subscription).hasValue(0) + } + + @Test + fun multipleSubscriptionsTracked() { + val m = ReflectionUtils.findMethod(SuspendingFunctions::class.java, "suspendingTracking", Continuation::class.java) + val reactiveTask = ReactiveTask(m!!, target!!, Duration.ZERO, Duration.ofMillis(500), false, true) + reactiveTask.subscribe() + Thread.sleep(1500) + reactiveTask.cancel() + assertThat(target!!.subscription).hasValueGreaterThanOrEqualTo(3) + } + + @Test + @Throws(InterruptedException::class) + fun noInitialDelayFixedDelay() { + val m = ReflectionUtils.findMethod(SuspendingFunctions::class.java, "suspendingTracking", Continuation::class.java) + val reactiveTask = ReactiveTask(m!!, target!!, Duration.ZERO, Duration.ofSeconds(10), false, true) + reactiveTask.subscribe() + Thread.sleep(500) + reactiveTask.cancel() + assertThat(target!!.subscription).hasValue(1) + } + + @Test + @Throws(InterruptedException::class) + fun noInitialDelayFixedRate() { + val m = ReflectionUtils.findMethod(SuspendingFunctions::class.java, "suspendingTracking", Continuation::class.java) + val reactiveTask = ReactiveTask(m!!, target!!, Duration.ZERO, Duration.ofSeconds(10), true, true) + reactiveTask.subscribe() + Thread.sleep(500) + reactiveTask.cancel() + assertThat(target!!.subscription).hasValue(1) + } + + @Test + @Throws(InterruptedException::class) + fun initialDelayFixedDelay() { + val m = ReflectionUtils.findMethod(SuspendingFunctions::class.java, "suspendingTracking", Continuation::class.java) + val reactiveTask = ReactiveTask(m!!, target!!, Duration.ofSeconds(10), Duration.ofMillis(500), false, true) + reactiveTask.subscribe() + Thread.sleep(500) + reactiveTask.cancel() + assertThat(target!!.subscription).hasValue(0) + } + + @Test + @Throws(InterruptedException::class) + fun initialDelayFixedRate() { + val m = ReflectionUtils.findMethod(SuspendingFunctions::class.java, "suspendingTracking", Continuation::class.java) + val reactiveTask = ReactiveTask(m!!, target!!, Duration.ofSeconds(10), Duration.ofMillis(500), true, true) + reactiveTask.subscribe() + Thread.sleep(500) + reactiveTask.cancel() + assertThat(target!!.subscription).hasValue(0) + } +} \ No newline at end of file From 2528ee3d50d34c2d16a2aae3f19f9013b0000158 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Basl=C3=A9?= Date: Tue, 14 Feb 2023 16:41:55 +0100 Subject: [PATCH 05/19] Polish assertion messages and checkpoint message --- .../ScheduledAnnotationReactiveSupport.java | 12 +++++++----- .../ScheduledAnnotationReactiveSupportTests.java | 11 +++++------ .../KotlinScheduledAnnotationReactiveSupportTests.kt | 10 +++++----- 3 files changed, 17 insertions(+), 16 deletions(-) diff --git a/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupport.java b/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupport.java index 73c64682f1f3..181d40344399 100644 --- a/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupport.java +++ b/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupport.java @@ -97,7 +97,8 @@ static boolean checkKotlinRuntimeIfNeeded(Method method) { * this method. */ static Publisher getPublisherForReactiveMethod(Method method, Object bean) { - Assert.isTrue(method.getParameterCount() == 0, "Only no-arg reactive methods may be annotated with @Scheduled"); + Assert.isTrue(method.getParameterCount() == 0, "Reactive methods may only be annotated with " + + "@Scheduled if declared without arguments"); Method invocableMethod = AopUtils.selectInvocableMethod(method, bean.getClass()); try { ReflectionUtils.makeAccessible(invocableMethod); @@ -119,8 +120,9 @@ static Publisher getPublisherForReactiveMethod(Method method, Object bean) { * this method. */ static Publisher getPublisherForSuspendingFunction(Method method, Object bean) { - Assert.isTrue(method.getParameterCount() == 1,"Only no-args suspending functions may be " - + "annotated with @Scheduled (with 1 self-referencing synthetic arg expected)"); + //Note that suspending functions declared without args have a single Continuation parameter in reflective inspection + Assert.isTrue(method.getParameterCount() == 1,"Kotlin suspending functions may only be annotated " + + "with @Scheduled if declared without arguments"); return CoroutinesUtils.invokeSuspendingFunction(method, bean, (Object[]) method.getParameters()); } @@ -154,8 +156,8 @@ protected ReactiveTask(Method method, Object bean, Duration initialDelay, this.isFixedRate = isFixedRate; this.disposable = Disposables.swap(); - this.checkpoint = "@Scheduled '"+ method.getDeclaringClass().getName() - + "#" + method.getName() + "()' [ScheduledAnnotationReactiveSupport]"; + this.checkpoint = "@Scheduled '"+ method.getName() + "()' in bean '" + + method.getDeclaringClass().getName() + "'"; } private Mono safeExecutionMono() { diff --git a/spring-context/src/test/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupportTests.java b/spring-context/src/test/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupportTests.java index 6b4a435c2dc5..747be22d3f90 100644 --- a/spring-context/src/test/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupportTests.java +++ b/spring-context/src/test/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupportTests.java @@ -141,13 +141,13 @@ void rejectWithParams() { //static helper method assertThatIllegalArgumentException().isThrownBy(() -> getPublisherForReactiveMethod(m, target)) - .withMessage("Only no-arg reactive methods may be annotated with @Scheduled") + .withMessage("Reactive methods may only be annotated with @Scheduled if declared without arguments") .withNoCause(); //constructor of task assertThatIllegalArgumentException().isThrownBy(() -> new ScheduledAnnotationReactiveSupport.ReactiveTask( m, target, Duration.ZERO, Duration.ZERO, false, false)) - .withMessage("Only no-arg reactive methods may be annotated with @Scheduled") + .withMessage("Reactive methods may only be annotated with @Scheduled if declared without arguments") .withNoCause(); } @@ -189,7 +189,7 @@ void hasCheckpointToString() { final ScheduledAnnotationReactiveSupport.ReactiveTask reactiveTask = new ScheduledAnnotationReactiveSupport.ReactiveTask( m, target, Duration.ZERO, Duration.ZERO, false, false); - assertThat(reactiveTask).hasToString("@Scheduled 'org.springframework.scheduling.annotation.ScheduledAnnotationReactiveSupportTests$ReactiveMethods#mono()' [ScheduledAnnotationReactiveSupport]"); + assertThat(reactiveTask).hasToString("@Scheduled 'mono()' in bean 'org.springframework.scheduling.annotation.ScheduledAnnotationReactiveSupportTests$ReactiveMethods'"); } @Test @@ -257,9 +257,8 @@ void monoErrorHasCheckpoint() throws InterruptedException { final ScheduledAnnotationReactiveSupport.ReactiveTask reactiveTask = new ScheduledAnnotationReactiveSupport.ReactiveTask( m, target, Duration.ZERO, Duration.ofSeconds(10), true, false); - assertThat(reactiveTask.checkpoint).isEqualTo("@Scheduled 'org.springframework.scheduling.annotation" - + ".ScheduledAnnotationReactiveSupportTests$ReactiveMethods#monoError()'" - + " [ScheduledAnnotationReactiveSupport]"); + assertThat(reactiveTask.checkpoint).isEqualTo("@Scheduled 'monoError()' in bean " + + "'org.springframework.scheduling.annotation.ScheduledAnnotationReactiveSupportTests$ReactiveMethods'"); } } diff --git a/spring-context/src/test/kotlin/org/springframework/scheduling/annotation/KotlinScheduledAnnotationReactiveSupportTests.kt b/spring-context/src/test/kotlin/org/springframework/scheduling/annotation/KotlinScheduledAnnotationReactiveSupportTests.kt index deae03db5e23..5c0e68cbaa1f 100644 --- a/spring-context/src/test/kotlin/org/springframework/scheduling/annotation/KotlinScheduledAnnotationReactiveSupportTests.kt +++ b/spring-context/src/test/kotlin/org/springframework/scheduling/annotation/KotlinScheduledAnnotationReactiveSupportTests.kt @@ -93,14 +93,14 @@ class KotlinScheduledAnnotationReactiveSupportTests { //static helper method Assertions.assertThatIllegalArgumentException().isThrownBy { getPublisherForSuspendingFunction(m!!, target!!) } - .withMessage("Only no-args suspending functions may be annotated with @Scheduled (with 1 self-referencing synthetic arg expected)") + .withMessage("Kotlin suspending functions may only be annotated with @Scheduled if declared without arguments") .withNoCause() //constructor of task Assertions.assertThatIllegalArgumentException().isThrownBy { ReactiveTask(m!!, target!!, Duration.ZERO, Duration.ZERO, false, true) } - .withMessage("Only no-args suspending functions may be annotated with @Scheduled (with 1 self-referencing synthetic arg expected)") + .withMessage("Kotlin suspending functions may only be annotated with @Scheduled if declared without arguments") .withNoCause() } @@ -110,14 +110,14 @@ class KotlinScheduledAnnotationReactiveSupportTests { //static helper method Assertions.assertThatIllegalArgumentException().isThrownBy { getPublisherForSuspendingFunction(m!!, target!!) } - .withMessage("Only no-args suspending functions may be annotated with @Scheduled (with 1 self-referencing synthetic arg expected)") + .withMessage("Kotlin suspending functions may only be annotated with @Scheduled if declared without arguments") .withNoCause() //constructor of task Assertions.assertThatIllegalArgumentException().isThrownBy { ReactiveTask(m!!, target!!, Duration.ZERO, Duration.ZERO, false, true) } - .withMessage("Only no-args suspending functions may be annotated with @Scheduled (with 1 self-referencing synthetic arg expected)") + .withMessage("Kotlin suspending functions may only be annotated with @Scheduled if declared without arguments") .withNoCause() } @@ -151,7 +151,7 @@ class KotlinScheduledAnnotationReactiveSupportTests { fun hasCheckpointToString() { val m = ReflectionUtils.findMethod(SuspendingFunctions::class.java, "suspending", Continuation::class.java) val reactiveTask = ReactiveTask(m!!, target!!, Duration.ZERO, Duration.ZERO, false, true) - assertThat(reactiveTask).hasToString("@Scheduled 'org.springframework.scheduling.annotation.KotlinScheduledAnnotationReactiveSupportTests\$SuspendingFunctions#suspending()' [ScheduledAnnotationReactiveSupport]") + assertThat(reactiveTask).hasToString("@Scheduled 'suspending()' in bean 'org.springframework.scheduling.annotation.KotlinScheduledAnnotationReactiveSupportTests\$SuspendingFunctions'") } @Test From 3a9c6e2ff0000fcdd781e8af2899407b4d3ea4be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Basl=C3=A9?= Date: Tue, 14 Feb 2023 17:04:12 +0100 Subject: [PATCH 06/19] Document reactive/kotlin support in javadoc of the annotations --- .../scheduling/annotation/EnableScheduling.java | 5 +++++ .../scheduling/annotation/Scheduled.java | 12 ++++++++++++ 2 files changed, 17 insertions(+) diff --git a/spring-context/src/main/java/org/springframework/scheduling/annotation/EnableScheduling.java b/spring-context/src/main/java/org/springframework/scheduling/annotation/EnableScheduling.java index 0d49e240d5df..d023651ef505 100644 --- a/spring-context/src/main/java/org/springframework/scheduling/annotation/EnableScheduling.java +++ b/spring-context/src/main/java/org/springframework/scheduling/annotation/EnableScheduling.java @@ -188,6 +188,11 @@ * application context and any separate {@code DispatcherServlet} application contexts, * if you need to apply its behavior at multiple levels. * + *

Note: {@code @EnableScheduling} and {@code @Scheduled} support of reactive methods + * and Kotlin suspending functions uses Reactor infrastructure instead of the + * {@code ScheduledTaskRegistrar} infrastructure, so previously discussed task-related + * aspects like {@code SchedulingConfigurer} don't apply to these two cases. + * * @author Chris Beams * @author Juergen Hoeller * @since 3.1 diff --git a/spring-context/src/main/java/org/springframework/scheduling/annotation/Scheduled.java b/spring-context/src/main/java/org/springframework/scheduling/annotation/Scheduled.java index 1b4cca077887..e589589a886d 100644 --- a/spring-context/src/main/java/org/springframework/scheduling/annotation/Scheduled.java +++ b/spring-context/src/main/java/org/springframework/scheduling/annotation/Scheduled.java @@ -36,6 +36,18 @@ * a {@code void} return type; if not, the returned value will be ignored * when called through the scheduler. * + *

Methods that return a reactive {@code Publisher} are supported provided the + * Reactor library is present at runtime: it is used to implement the scheduling by + * repeatedly subscribing to the returned Publisher, which is only produced once. + * The cron configuration is not supported for this type of method. Values emitted by + * the publisher are ignored and discarded. Errors are logged at WARN level, which + * doesn't prevent further iterations. + * + *

Kotlin suspending functions are also supported, provided the coroutine-Reactor + * bridge ({@code kotlinx.coroutine.reactor}) is present at runtime. This bridge is + * used to adapt the suspending function into a Reactor {@code Mono} which is treated + * the same way as in the reactive method case (see above). + * *

Processing of {@code @Scheduled} annotations is performed by * registering a {@link ScheduledAnnotationBeanPostProcessor}. This can be * done manually or, more conveniently, through the {@code } From 837b76c3a3dedeaadb8a1aabe4bdd2618273ca2a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Basl=C3=A9?= Date: Tue, 14 Feb 2023 17:49:15 +0100 Subject: [PATCH 07/19] Document reactive Scheduled support in the reference guide --- .../ROOT/pages/integration/scheduling.adoc | 81 +++++++++++++++++++ 1 file changed, 81 insertions(+) diff --git a/framework-docs/modules/ROOT/pages/integration/scheduling.adoc b/framework-docs/modules/ROOT/pages/integration/scheduling.adoc index eae7ff2e7045..4da17e246348 100644 --- a/framework-docs/modules/ROOT/pages/integration/scheduling.adoc +++ b/framework-docs/modules/ROOT/pages/integration/scheduling.adoc @@ -393,6 +393,87 @@ container and once through the `@Configurable` aspect), with the consequence of `@Scheduled` method being invoked twice. ==== +[[scheduling-annotation-support-scheduled-reactive]] +=== The `@Scheduled` annotation on Reactive methods or Kotlin suspending functions + +As of Spring Framework 6.1, `@Scheduled` methods are also supported on two types +of asynchronous methods: + + - Reactive methods, i.e. methods with a `Publisher` return type (or any concrete +implementation of `Publisher`) like in the following example: + +[source,java,indent=0,subs="verbatim,quotes"] +---- + @Scheduled(fixedDelay = 500) + public Publisher reactiveSomething() { + // return an instance of Publisher + } +---- + + - Kotlin suspending functions, like in the following example: + +[source,kotlin,indent=0,subs="verbatim,quotes"] +---- + @Scheduled(fixedDelay = 500) + suspend fun something() { + // do something asynchronous + } +---- + +Both types of methods must be declared without any arguments. Additionally, the `cron` +configuration elements are not supported for these two cases. + +Note that the `TaskExecutor`/`TaskScheduler` infrastructure is not used and the scheduling +is instead performed by the Reactor library, which must be present at runtime. +In the case of Kotlin suspending functions the `kotlinx.coroutines.reactor` bridge must +also be present. + +The Spring Framework will obtain a `Publisher` out of the annotated method once and will +create a timed `Flux` which regularly subscribes to said `Publisher`. These inner regular +subscriptions happen according to the `fixedDelay`/`fixedRate` configuration. + +If the `Publisher` emits `onNext` signal(s), these are ignored and discarded (the same way +return values from synchronous `@Scheduled` methods are ignored). + +In the following example, the `Flux` emits `onNext("Hello"), onNext("World")` every 5 +seconds, but these values are unused: + +[source,java,indent=0,subs="verbatim,quotes"] +---- + @Scheduled(initialDelay = 5000, fixedRate = 5000) + public Flux reactiveSomething() { + return Flux.just("Hello", "World"); + } +---- + +If the `Publisher` emits an `onError` signal, it is logged at WARN level and recovered. +As a result, further scheduled subscription do happen despite the error. + +In the following example, the `Mono` subscription fails twice in the first five seconds +then subscriptions start succeeding, printing a message to the standard output every five +seconds: + +[source,java,indent=0,subs="verbatim,quotes"] +---- + @Scheduled(initialDelay = 0, fixedRate = 5000) + public Mono reactiveSomething() { + AtomicInteger countdown = new AtomicInteger(2); + + return Mono.defer(() -> { + if (countDown.get() == 0 || countDown.decrementAndGet() == 0) { + return Mono.fromRunnable(() -> System.out.println("Message")); + } + return Mono.error(new IllegalStateException("Cannot deliver message")); + }) + } +---- + +[NOTE] +==== +The Spring Framework subscribes once to the underlying scheduling `Flux`, which can be +cancelled by destroying the annotated bean or closing the application context. +==== + [[scheduling-annotation-support-async]] === The `@Async` annotation From 5d62e18e546e80e31086dfe7cb6a24d617b68695 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Basl=C3=A9?= Date: Tue, 14 Feb 2023 18:01:33 +0100 Subject: [PATCH 08/19] fix unclosed b tag in javadoc --- .../springframework/scheduling/annotation/EnableScheduling.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spring-context/src/main/java/org/springframework/scheduling/annotation/EnableScheduling.java b/spring-context/src/main/java/org/springframework/scheduling/annotation/EnableScheduling.java index d023651ef505..5fd0f049a34b 100644 --- a/spring-context/src/main/java/org/springframework/scheduling/annotation/EnableScheduling.java +++ b/spring-context/src/main/java/org/springframework/scheduling/annotation/EnableScheduling.java @@ -189,7 +189,7 @@ * if you need to apply its behavior at multiple levels. * *

Note: {@code @EnableScheduling} and {@code @Scheduled} support of reactive methods - * and Kotlin suspending functions uses Reactor infrastructure instead of the + * and Kotlin suspending functions uses Reactor infrastructure instead of the * {@code ScheduledTaskRegistrar} infrastructure, so previously discussed task-related * aspects like {@code SchedulingConfigurer} don't apply to these two cases. * From c90595e4032ec681c9ffea1620ae024dbb873a40 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Basl=C3=A9?= Date: Thu, 16 Feb 2023 16:28:41 +0100 Subject: [PATCH 09/19] Polish: use a single check for reactive and kotlin methods The support's `isReactive` method checks Kotlin suspending functions first then reactive (Publisher-returning) methods second. It asserts the relevant runtime, all in a single call. Similarly, turning the Method into a Publisher is done via a single common helper method `getPublisherFor(Method, Object)`. All imports of reactive classes and other reactive-specific logic is still outsourced to ScheduledAnnotationReactiveSupport in order to avoid any classpath issue in the bean postprocessor. --- .../ScheduledAnnotationBeanPostProcessor.java | 22 ++--- .../ScheduledAnnotationReactiveSupport.java | 92 +++++++++---------- ...heduledAnnotationReactiveSupportTests.java | 36 ++++---- ...ScheduledAnnotationReactiveSupportTests.kt | 39 ++++---- 4 files changed, 87 insertions(+), 102 deletions(-) diff --git a/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationBeanPostProcessor.java b/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationBeanPostProcessor.java index 8cd127a39927..4846a3304e98 100644 --- a/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationBeanPostProcessor.java +++ b/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationBeanPostProcessor.java @@ -396,14 +396,10 @@ public Object postProcessAfterInitialization(Object bean, String beanName) { } protected void processScheduled(Scheduled scheduled, Method method, Object bean) { - // Check for Kotlin suspending functions. If one is found but reactor bridge isn't on the classpath, throw - if (ScheduledAnnotationReactiveSupport.checkKotlinRuntimeIfNeeded(method)) { - processScheduledReactive(scheduled, method, bean, true); - return; - } - // Check for Publisher-returning methods. If found but Reactor isn't on the classpath, throw. - if (ScheduledAnnotationReactiveSupport.checkReactorRuntimeIfNeeded(method)) { - processScheduledReactive(scheduled, method, bean, false); + // Is method a Kotlin suspending function? Throws if true but reactor bridge isn't on the classpath. + // Is method returning a Publisher instance? Throws if true but Reactor isn't on the classpath. + if (ScheduledAnnotationReactiveSupport.isReactive(method)) { + processScheduledReactive(scheduled, method, bean); return; } processScheduledSync(scheduled, method, bean); @@ -553,7 +549,7 @@ protected void processScheduledSync(Scheduled scheduled, Method method, Object b * @param bean the target bean instance * @see #createRunnable(Object, Method) */ - protected void processScheduledReactive(Scheduled scheduled, Method method, Object bean, boolean isSuspendingFunction) { + protected void processScheduledReactive(Scheduled scheduled, Method method, Object bean) { try { boolean processedSchedule = false; String errorMessage = @@ -592,7 +588,7 @@ protected void processScheduledReactive(Scheduled scheduled, Method method, Obje Duration fixedDelay = toDuration(scheduled.fixedDelay(), scheduled.timeUnit()); if (!fixedDelay.isNegative()) { processedSchedule = true; - reactiveTasks.add(new ScheduledAnnotationReactiveSupport.ReactiveTask(method, bean, initialDelay, fixedDelay, false, isSuspendingFunction)); + reactiveTasks.add(new ScheduledAnnotationReactiveSupport.ReactiveTask(method, bean, initialDelay, fixedDelay, false)); } String fixedDelayString = scheduled.fixedDelayString(); @@ -610,7 +606,7 @@ protected void processScheduledReactive(Scheduled scheduled, Method method, Obje throw new IllegalArgumentException( "Invalid fixedDelayString value \"" + fixedDelayString + "\" - cannot parse into long"); } - reactiveTasks.add(new ScheduledAnnotationReactiveSupport.ReactiveTask(method, bean, initialDelay, fixedDelay, false, isSuspendingFunction)); + reactiveTasks.add(new ScheduledAnnotationReactiveSupport.ReactiveTask(method, bean, initialDelay, fixedDelay, false)); } } @@ -619,7 +615,7 @@ protected void processScheduledReactive(Scheduled scheduled, Method method, Obje if (!fixedRate.isNegative()) { Assert.isTrue(!processedSchedule, errorMessage); processedSchedule = true; - reactiveTasks.add(new ScheduledAnnotationReactiveSupport.ReactiveTask(method, bean, initialDelay, fixedRate, true, isSuspendingFunction)); + reactiveTasks.add(new ScheduledAnnotationReactiveSupport.ReactiveTask(method, bean, initialDelay, fixedRate, true)); } String fixedRateString = scheduled.fixedRateString(); if (StringUtils.hasText(fixedRateString)) { @@ -636,7 +632,7 @@ protected void processScheduledReactive(Scheduled scheduled, Method method, Obje throw new IllegalArgumentException( "Invalid fixedRateString value \"" + fixedRateString + "\" - cannot parse into long"); } - reactiveTasks.add(new ScheduledAnnotationReactiveSupport.ReactiveTask(method, bean, initialDelay, fixedRate, true, isSuspendingFunction)); + reactiveTasks.add(new ScheduledAnnotationReactiveSupport.ReactiveTask(method, bean, initialDelay, fixedRate, true)); } } diff --git a/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupport.java b/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupport.java index 181d40344399..f9b5762be34a 100644 --- a/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupport.java +++ b/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupport.java @@ -39,7 +39,7 @@ * Helper class for @{@link ScheduledAnnotationBeanPostProcessor} to support reactive cases * without a dependency on optional classes. * @author Simon Baslé - * @since 6.0.x //FIXME + * @since 6.1.0 //FIXME */ abstract class ScheduledAnnotationReactiveSupport { @@ -53,17 +53,35 @@ abstract class ScheduledAnnotationReactiveSupport { "kotlinx.coroutines.reactor.MonoKt", ScheduledAnnotationReactiveSupport.class.getClassLoader()); /** - * Checks that if the method is reactive, it can be scheduled. If it isn't reactive - * (Reactive Streams {@code Publisher} is not present at runtime or the method doesn't - * return a form of Publisher) then this check returns {@code false}. Otherwise, it is - * eligible for reactive scheduling and Reactor MUST also be present at runtime. + * Checks that if the method is reactive, it can be scheduled. Methods are considered + * eligible for reactive scheduling if they either return an instance of + * {@code Publisher} or are a Kotlin Suspending Function. If the method isn't matching + * these criteria then this check returns {@code false}. + *

For reactive scheduling, Reactor MUST be present at runtime. In the case of + * Kotlin, the Coroutine {@code kotlinx.coroutines.reactor} bridge MUST also be + * present at runtime (in order to invoke suspending functions as a {@code Mono}). * Provided that is the case, this method returns {@code true}. Otherwise, it throws * an {@code IllegalStateException}. + * @throws IllegalStateException if the method is reactive but Reactor and/or the + * Kotlin coroutines bridge are not present at runtime */ - static boolean checkReactorRuntimeIfNeeded(Method method) { - if (!(publisherPresent && Publisher.class.isAssignableFrom(method.getReturnType()))) { + static boolean isReactive(Method method) { + if (KotlinDetector.isKotlinPresent() && KotlinDetector.isSuspendingFunction(method)) { + if (coroutinesReactorPresent) { + return true; + } + else { + throw new IllegalStateException("Kotlin suspending functions may only be annotated with @Scheduled" + + "if Reactor and the Reactor-Coroutine bridge (kotlinx.coroutines.reactor) are present at runtime"); + } + } + if (!publisherPresent) { + return false; + } + if (!Publisher.class.isAssignableFrom(method.getReturnType())) { return false; } + if (reactorPresent) { return true; } @@ -72,31 +90,21 @@ static boolean checkReactorRuntimeIfNeeded(Method method) { } /** - * Checks that if the method is a Kotlin suspending function, it can be scheduled. - * If it isn't a suspending function (or Kotlin is not detected at runtime) then this - * check returns {@code false}. Otherwise, it is eligible for conversion to a - * {@code Mono} for reactive scheduling and both Reactor and the - * {@code kotlinx.coroutines.reactor} bridge MUST also be present at runtime. - * Provided that is the case, this method returns {@code true}. Otherwise, it throws - * an {@code IllegalStateException}. + * Turn the invocation of the provided {@code Method} into a {@code Publisher}, + * either by reflectively invoking it and returning the resulting {@code Publisher} + * or by converting a Kotlin suspending function into a Publisher via + * {@link CoroutinesUtils}. + * The {@link #isReactive(Method)} check is a precondition to calling this method. */ - static boolean checkKotlinRuntimeIfNeeded(Method method) { - if (!(KotlinDetector.isKotlinPresent() && KotlinDetector.isSuspendingFunction(method))) { - return false; - } - if (coroutinesReactorPresent) { - return true; + static Publisher getPublisherFor(Method method, Object bean) { + if (KotlinDetector.isKotlinPresent() && KotlinDetector.isSuspendingFunction(method)) { + //Note that suspending functions declared without args have a single Continuation parameter in reflective inspection + Assert.isTrue(method.getParameterCount() == 1,"Kotlin suspending functions may only be annotated " + + "with @Scheduled if declared without arguments"); + + return CoroutinesUtils.invokeSuspendingFunction(method, bean, (Object[]) method.getParameters()); } - throw new IllegalStateException("Kotlin suspending functions may only be annotated with @Scheduled" - + "if Reactor and the Reactor-Coroutine bridge (kotlinx.coroutines.reactor) are present at runtime"); - } - /** - * Reflectively invoke a reactive method and return the resulting {@code Publisher}. - * The {@link #checkReactorRuntimeIfNeeded(Method)} check is a precondition to calling - * this method. - */ - static Publisher getPublisherForReactiveMethod(Method method, Object bean) { Assert.isTrue(method.getParameterCount() == 0, "Reactive methods may only be annotated with " + "@Scheduled if declared without arguments"); Method invocableMethod = AopUtils.selectInvocableMethod(method, bean.getClass()); @@ -113,22 +121,10 @@ static Publisher getPublisherForReactiveMethod(Method method, Object bean) { } } - /** - * Turn the provided suspending function method into a {@code Publisher} via - * {@link CoroutinesUtils} and return that Publisher. - * The {@link #checkKotlinRuntimeIfNeeded(Method)} check is a precondition to calling - * this method. - */ - static Publisher getPublisherForSuspendingFunction(Method method, Object bean) { - //Note that suspending functions declared without args have a single Continuation parameter in reflective inspection - Assert.isTrue(method.getParameterCount() == 1,"Kotlin suspending functions may only be annotated " - + "with @Scheduled if declared without arguments"); - - return CoroutinesUtils.invokeSuspendingFunction(method, bean, (Object[]) method.getParameters()); - } - /** * Encapsulates the logic of {@code @Scheduled} on reactive types, using Reactor. + * The {@link ScheduledAnnotationReactiveSupport#isReactive(Method)} check is a + * precondition to instantiating this class. */ static class ReactiveTask { @@ -142,14 +138,8 @@ static class ReactiveTask { private final Log logger = LogFactory.getLog(getClass()); - protected ReactiveTask(Method method, Object bean, Duration initialDelay, - Duration otherDelay, boolean isFixedRate, boolean isSuspendingFunction) { - if (isSuspendingFunction) { - this.publisher = getPublisherForSuspendingFunction(method, bean); - } - else { - this.publisher = getPublisherForReactiveMethod(method, bean); - } + protected ReactiveTask(Method method, Object bean, Duration initialDelay, Duration otherDelay, boolean isFixedRate) { + this.publisher = getPublisherFor(method, bean); this.initialDelay = initialDelay; this.otherDelay = otherDelay; diff --git a/spring-context/src/test/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupportTests.java b/spring-context/src/test/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupportTests.java index 747be22d3f90..0b6d56550a21 100644 --- a/spring-context/src/test/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupportTests.java +++ b/spring-context/src/test/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupportTests.java @@ -34,7 +34,7 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; -import static org.springframework.scheduling.annotation.ScheduledAnnotationReactiveSupport.getPublisherForReactiveMethod; +import static org.springframework.scheduling.annotation.ScheduledAnnotationReactiveSupport.getPublisherFor; class ScheduledAnnotationReactiveSupportTests { @@ -48,7 +48,7 @@ void ensureReactor() { "publisherString", "monoThrows" }) //note: monoWithParams can't be found by this test void checkIsReactive(String method) { Method m = ReflectionUtils.findMethod(ReactiveMethods.class, method); - assertThat(ScheduledAnnotationReactiveSupport.checkReactorRuntimeIfNeeded(m)).as(m.getName()).isTrue(); + assertThat(ScheduledAnnotationReactiveSupport.isReactive(m)).as(m.getName()).isTrue(); } @Test @@ -56,9 +56,9 @@ void checkNotReactive() { Method string = ReflectionUtils.findMethod(ReactiveMethods.class, "oops"); Method future = ReflectionUtils.findMethod(ReactiveMethods.class, "future"); - assertThat(ScheduledAnnotationReactiveSupport.checkReactorRuntimeIfNeeded(string)) + assertThat(ScheduledAnnotationReactiveSupport.isReactive(string)) .as("String-returning").isFalse(); - assertThat(ScheduledAnnotationReactiveSupport.checkReactorRuntimeIfNeeded(future)) + assertThat(ScheduledAnnotationReactiveSupport.isReactive(future)) .as("Future-returning").isFalse(); } @@ -137,16 +137,16 @@ void init() { void rejectWithParams() { Method m = ReflectionUtils.findMethod(ReactiveMethods.class, "monoWithParam", String.class); - assertThat(ScheduledAnnotationReactiveSupport.checkReactorRuntimeIfNeeded(m)).as("isReactive").isTrue(); + assertThat(ScheduledAnnotationReactiveSupport.isReactive(m)).as("isReactive").isTrue(); //static helper method - assertThatIllegalArgumentException().isThrownBy(() -> getPublisherForReactiveMethod(m, target)) + assertThatIllegalArgumentException().isThrownBy(() -> getPublisherFor(m, target)) .withMessage("Reactive methods may only be annotated with @Scheduled if declared without arguments") .withNoCause(); //constructor of task assertThatIllegalArgumentException().isThrownBy(() -> new ScheduledAnnotationReactiveSupport.ReactiveTask( - m, target, Duration.ZERO, Duration.ZERO, false, false)) + m, target, Duration.ZERO, Duration.ZERO, false)) .withMessage("Reactive methods may only be annotated with @Scheduled if declared without arguments") .withNoCause(); } @@ -156,13 +156,13 @@ void rejectCantProducePublisher() { Method m = ReflectionUtils.findMethod(ReactiveMethods.class, "monoThrows"); //static helper method - assertThatIllegalArgumentException().isThrownBy(() -> getPublisherForReactiveMethod(m, target)) + assertThatIllegalArgumentException().isThrownBy(() -> getPublisherFor(m, target)) .withMessage("Cannot obtain a Publisher from the @Scheduled reactive method") .withCause(new IllegalStateException("expected")); //constructor of task assertThatIllegalArgumentException().isThrownBy(() -> new ScheduledAnnotationReactiveSupport.ReactiveTask( - m, target, Duration.ZERO, Duration.ZERO, false, false)) + m, target, Duration.ZERO, Duration.ZERO, false)) .withMessage("Cannot obtain a Publisher from the @Scheduled reactive method") .withCause(new IllegalStateException("expected")); } @@ -172,13 +172,13 @@ void rejectCantAccessMethod() { Method m = ReflectionUtils.findMethod(ReactiveMethods.class, "monoThrowsIllegalAccess"); //static helper method - assertThatIllegalArgumentException().isThrownBy(() -> getPublisherForReactiveMethod(m, target)) + assertThatIllegalArgumentException().isThrownBy(() -> getPublisherFor(m, target)) .withMessage("Cannot obtain a Publisher from the @Scheduled reactive method") .withCause(new IllegalAccessException("expected")); //constructor of task assertThatIllegalArgumentException().isThrownBy(() -> new ScheduledAnnotationReactiveSupport.ReactiveTask( - m, target, Duration.ZERO, Duration.ZERO, false, false)) + m, target, Duration.ZERO, Duration.ZERO, false)) .withMessage("Cannot obtain a Publisher from the @Scheduled reactive method") .withCause(new IllegalAccessException("expected")); } @@ -187,7 +187,7 @@ void rejectCantAccessMethod() { void hasCheckpointToString() { Method m = ReflectionUtils.findMethod(ReactiveMethods.class, "mono"); final ScheduledAnnotationReactiveSupport.ReactiveTask reactiveTask = new ScheduledAnnotationReactiveSupport.ReactiveTask( - m, target, Duration.ZERO, Duration.ZERO, false, false); + m, target, Duration.ZERO, Duration.ZERO, false); assertThat(reactiveTask).hasToString("@Scheduled 'mono()' in bean 'org.springframework.scheduling.annotation.ScheduledAnnotationReactiveSupportTests$ReactiveMethods'"); } @@ -196,7 +196,7 @@ void hasCheckpointToString() { void cancelledEarlyPreventsSubscription() { Method m = ReflectionUtils.findMethod(ReactiveMethods.class, "trackingMono"); final ScheduledAnnotationReactiveSupport.ReactiveTask reactiveTask = new ScheduledAnnotationReactiveSupport.ReactiveTask( - m, target, Duration.ZERO, Duration.ofSeconds(10), false, false); + m, target, Duration.ZERO, Duration.ofSeconds(10), false); reactiveTask.cancel(); reactiveTask.subscribe(); @@ -207,7 +207,7 @@ void cancelledEarlyPreventsSubscription() { void noInitialDelayFixedDelay() throws InterruptedException { Method m = ReflectionUtils.findMethod(target.getClass(), "trackingMono"); final ScheduledAnnotationReactiveSupport.ReactiveTask reactiveTask = new ScheduledAnnotationReactiveSupport.ReactiveTask( - m, target, Duration.ZERO, Duration.ofSeconds(10), false, false); + m, target, Duration.ZERO, Duration.ofSeconds(10), false); reactiveTask.subscribe(); Thread.sleep(500); reactiveTask.cancel(); @@ -219,7 +219,7 @@ void noInitialDelayFixedDelay() throws InterruptedException { void noInitialDelayFixedRate() throws InterruptedException { Method m = ReflectionUtils.findMethod(target.getClass(), "trackingMono"); final ScheduledAnnotationReactiveSupport.ReactiveTask reactiveTask = new ScheduledAnnotationReactiveSupport.ReactiveTask( - m, target, Duration.ZERO, Duration.ofSeconds(10), true, false); + m, target, Duration.ZERO, Duration.ofSeconds(10), true); reactiveTask.subscribe(); Thread.sleep(500); reactiveTask.cancel(); @@ -231,7 +231,7 @@ void noInitialDelayFixedRate() throws InterruptedException { void initialDelayFixedDelay() throws InterruptedException { Method m = ReflectionUtils.findMethod(target.getClass(), "trackingMono"); final ScheduledAnnotationReactiveSupport.ReactiveTask reactiveTask = new ScheduledAnnotationReactiveSupport.ReactiveTask( - m, target, Duration.ofSeconds(10), Duration.ofMillis(500), false, false); + m, target, Duration.ofSeconds(10), Duration.ofMillis(500), false); reactiveTask.subscribe(); Thread.sleep(500); reactiveTask.cancel(); @@ -243,7 +243,7 @@ void initialDelayFixedDelay() throws InterruptedException { void initialDelayFixedRate() throws InterruptedException { Method m = ReflectionUtils.findMethod(target.getClass(), "trackingMono"); final ScheduledAnnotationReactiveSupport.ReactiveTask reactiveTask = new ScheduledAnnotationReactiveSupport.ReactiveTask( - m, target, Duration.ofSeconds(10), Duration.ofMillis(500), true, false); + m, target, Duration.ofSeconds(10), Duration.ofMillis(500), true); reactiveTask.subscribe(); Thread.sleep(500); reactiveTask.cancel(); @@ -255,7 +255,7 @@ void initialDelayFixedRate() throws InterruptedException { void monoErrorHasCheckpoint() throws InterruptedException { Method m = ReflectionUtils.findMethod(target.getClass(), "monoError"); final ScheduledAnnotationReactiveSupport.ReactiveTask reactiveTask = new ScheduledAnnotationReactiveSupport.ReactiveTask( - m, target, Duration.ZERO, Duration.ofSeconds(10), true, false); + m, target, Duration.ZERO, Duration.ofSeconds(10), true); assertThat(reactiveTask.checkpoint).isEqualTo("@Scheduled 'monoError()' in bean " + "'org.springframework.scheduling.annotation.ScheduledAnnotationReactiveSupportTests$ReactiveMethods'"); diff --git a/spring-context/src/test/kotlin/org/springframework/scheduling/annotation/KotlinScheduledAnnotationReactiveSupportTests.kt b/spring-context/src/test/kotlin/org/springframework/scheduling/annotation/KotlinScheduledAnnotationReactiveSupportTests.kt index 5c0e68cbaa1f..2fe8e0302054 100644 --- a/spring-context/src/test/kotlin/org/springframework/scheduling/annotation/KotlinScheduledAnnotationReactiveSupportTests.kt +++ b/spring-context/src/test/kotlin/org/springframework/scheduling/annotation/KotlinScheduledAnnotationReactiveSupportTests.kt @@ -21,9 +21,8 @@ import org.assertj.core.api.Assertions.assertThat import org.junit.jupiter.api.BeforeEach import org.junit.jupiter.api.Test import org.springframework.scheduling.annotation.ScheduledAnnotationReactiveSupport.ReactiveTask -import org.springframework.scheduling.annotation.ScheduledAnnotationReactiveSupport.checkKotlinRuntimeIfNeeded -import org.springframework.scheduling.annotation.ScheduledAnnotationReactiveSupport.getPublisherForSuspendingFunction -import org.springframework.scheduling.annotation.ScheduledAnnotationReactiveSupportTests.ReactiveMethods +import org.springframework.scheduling.annotation.ScheduledAnnotationReactiveSupport.getPublisherFor +import org.springframework.scheduling.annotation.ScheduledAnnotationReactiveSupport.isReactive import org.springframework.util.ReflectionUtils import reactor.core.publisher.Mono import java.time.Duration @@ -45,7 +44,7 @@ class KotlinScheduledAnnotationReactiveSupportTests { @Test fun kotlinSuspendingFunctionIsNotReactive() { val suspending = ReflectionUtils.findMethod(SuspendingFunctions::class.java, "suspending", Continuation::class.java) - Assertions.assertThat(ScheduledAnnotationReactiveSupport.checkReactorRuntimeIfNeeded(suspending!!)).isFalse + Assertions.assertThat(isReactive(suspending!!)).isFalse } internal class SuspendingFunctions { @@ -83,8 +82,8 @@ class KotlinScheduledAnnotationReactiveSupportTests { val suspendingMethod = ReflectionUtils.findMethod(SuspendingFunctions::class.java, "suspending", Continuation::class.java)!! val notSuspendingMethod = ReflectionUtils.findMethod(SuspendingFunctions::class.java, "notSuspending")!! - assertThat(checkKotlinRuntimeIfNeeded(suspendingMethod)).describedAs("suspending").isTrue() - assertThat(checkKotlinRuntimeIfNeeded(notSuspendingMethod)).describedAs("not suspending").isFalse() + assertThat(isReactive(suspendingMethod)).describedAs("suspending").isTrue() + assertThat(isReactive(notSuspendingMethod)).describedAs("not suspending").isFalse() } @Test @@ -92,13 +91,13 @@ class KotlinScheduledAnnotationReactiveSupportTests { val m = ReflectionUtils.findMethod(SuspendingFunctions::class.java, "withParam", String::class.java, Continuation::class.java) //static helper method - Assertions.assertThatIllegalArgumentException().isThrownBy { getPublisherForSuspendingFunction(m!!, target!!) } + Assertions.assertThatIllegalArgumentException().isThrownBy { getPublisherFor(m!!, target!!) } .withMessage("Kotlin suspending functions may only be annotated with @Scheduled if declared without arguments") .withNoCause() //constructor of task Assertions.assertThatIllegalArgumentException().isThrownBy { - ReactiveTask(m!!, target!!, Duration.ZERO, Duration.ZERO, false, true) + ReactiveTask(m!!, target!!, Duration.ZERO, Duration.ZERO, false) } .withMessage("Kotlin suspending functions may only be annotated with @Scheduled if declared without arguments") .withNoCause() @@ -109,13 +108,13 @@ class KotlinScheduledAnnotationReactiveSupportTests { val m = ReflectionUtils.findMethod(SuspendingFunctions::class.java, "notSuspending") //static helper method - Assertions.assertThatIllegalArgumentException().isThrownBy { getPublisherForSuspendingFunction(m!!, target!!) } + Assertions.assertThatIllegalArgumentException().isThrownBy { getPublisherFor(m!!, target!!) } .withMessage("Kotlin suspending functions may only be annotated with @Scheduled if declared without arguments") .withNoCause() //constructor of task Assertions.assertThatIllegalArgumentException().isThrownBy { - ReactiveTask(m!!, target!!, Duration.ZERO, Duration.ZERO, false, true) + ReactiveTask(m!!, target!!, Duration.ZERO, Duration.ZERO, false) } .withMessage("Kotlin suspending functions may only be annotated with @Scheduled if declared without arguments") .withNoCause() @@ -125,7 +124,7 @@ class KotlinScheduledAnnotationReactiveSupportTests { fun suspendingThrowIsTurnedToMonoError() { val m = ReflectionUtils.findMethod(SuspendingFunctions::class.java, "throwsIllegalState", Continuation::class.java) - val mono = Mono.from(getPublisherForSuspendingFunction(m!!, target!!)) + val mono = Mono.from(getPublisherFor(m!!, target!!)) Assertions.assertThatIllegalStateException().isThrownBy { mono.block() } .withMessage("expected") @@ -133,14 +132,14 @@ class KotlinScheduledAnnotationReactiveSupportTests { //constructor of task doesn't throw Assertions.assertThatNoException().isThrownBy { - ReactiveTask(m, target!!, Duration.ZERO, Duration.ZERO, false, true) + ReactiveTask(m, target!!, Duration.ZERO, Duration.ZERO, false) } } @Test fun turningSuspendingFunctionToMonoDoesntExecuteTheMethod() { val m = ReflectionUtils.findMethod(SuspendingFunctions::class.java, "suspendingTracking", Continuation::class.java) - val mono = Mono.from(getPublisherForSuspendingFunction(m!!, target!!)) + val mono = Mono.from(getPublisherFor(m!!, target!!)) assertThat(target!!.subscription).hasValue(0) mono.block() @@ -150,14 +149,14 @@ class KotlinScheduledAnnotationReactiveSupportTests { @Test fun hasCheckpointToString() { val m = ReflectionUtils.findMethod(SuspendingFunctions::class.java, "suspending", Continuation::class.java) - val reactiveTask = ReactiveTask(m!!, target!!, Duration.ZERO, Duration.ZERO, false, true) + val reactiveTask = ReactiveTask(m!!, target!!, Duration.ZERO, Duration.ZERO, false) assertThat(reactiveTask).hasToString("@Scheduled 'suspending()' in bean 'org.springframework.scheduling.annotation.KotlinScheduledAnnotationReactiveSupportTests\$SuspendingFunctions'") } @Test fun cancelledEarlyPreventsSubscription() { val m = ReflectionUtils.findMethod(SuspendingFunctions::class.java, "suspendingTracking", Continuation::class.java) - val reactiveTask = ReactiveTask(m!!, target!!, Duration.ZERO, Duration.ofSeconds(10), false, true) + val reactiveTask = ReactiveTask(m!!, target!!, Duration.ZERO, Duration.ofSeconds(10), false) reactiveTask.cancel() reactiveTask.subscribe() assertThat(target!!.subscription).hasValue(0) @@ -166,7 +165,7 @@ class KotlinScheduledAnnotationReactiveSupportTests { @Test fun multipleSubscriptionsTracked() { val m = ReflectionUtils.findMethod(SuspendingFunctions::class.java, "suspendingTracking", Continuation::class.java) - val reactiveTask = ReactiveTask(m!!, target!!, Duration.ZERO, Duration.ofMillis(500), false, true) + val reactiveTask = ReactiveTask(m!!, target!!, Duration.ZERO, Duration.ofMillis(500), false) reactiveTask.subscribe() Thread.sleep(1500) reactiveTask.cancel() @@ -177,7 +176,7 @@ class KotlinScheduledAnnotationReactiveSupportTests { @Throws(InterruptedException::class) fun noInitialDelayFixedDelay() { val m = ReflectionUtils.findMethod(SuspendingFunctions::class.java, "suspendingTracking", Continuation::class.java) - val reactiveTask = ReactiveTask(m!!, target!!, Duration.ZERO, Duration.ofSeconds(10), false, true) + val reactiveTask = ReactiveTask(m!!, target!!, Duration.ZERO, Duration.ofSeconds(10), false) reactiveTask.subscribe() Thread.sleep(500) reactiveTask.cancel() @@ -188,7 +187,7 @@ class KotlinScheduledAnnotationReactiveSupportTests { @Throws(InterruptedException::class) fun noInitialDelayFixedRate() { val m = ReflectionUtils.findMethod(SuspendingFunctions::class.java, "suspendingTracking", Continuation::class.java) - val reactiveTask = ReactiveTask(m!!, target!!, Duration.ZERO, Duration.ofSeconds(10), true, true) + val reactiveTask = ReactiveTask(m!!, target!!, Duration.ZERO, Duration.ofSeconds(10), true) reactiveTask.subscribe() Thread.sleep(500) reactiveTask.cancel() @@ -199,7 +198,7 @@ class KotlinScheduledAnnotationReactiveSupportTests { @Throws(InterruptedException::class) fun initialDelayFixedDelay() { val m = ReflectionUtils.findMethod(SuspendingFunctions::class.java, "suspendingTracking", Continuation::class.java) - val reactiveTask = ReactiveTask(m!!, target!!, Duration.ofSeconds(10), Duration.ofMillis(500), false, true) + val reactiveTask = ReactiveTask(m!!, target!!, Duration.ofSeconds(10), Duration.ofMillis(500), false) reactiveTask.subscribe() Thread.sleep(500) reactiveTask.cancel() @@ -210,7 +209,7 @@ class KotlinScheduledAnnotationReactiveSupportTests { @Throws(InterruptedException::class) fun initialDelayFixedRate() { val m = ReflectionUtils.findMethod(SuspendingFunctions::class.java, "suspendingTracking", Continuation::class.java) - val reactiveTask = ReactiveTask(m!!, target!!, Duration.ofSeconds(10), Duration.ofMillis(500), true, true) + val reactiveTask = ReactiveTask(m!!, target!!, Duration.ofSeconds(10), Duration.ofMillis(500), true) reactiveTask.subscribe() Thread.sleep(500) reactiveTask.cancel() From af9c88c07f6ca3e382a3a9e60bfdf9ce514eab0d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Basl=C3=A9?= Date: Mon, 20 Feb 2023 10:21:25 +0100 Subject: [PATCH 10/19] polish javadocs --- .../ScheduledAnnotationBeanPostProcessor.java | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationBeanPostProcessor.java b/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationBeanPostProcessor.java index 4846a3304e98..f3bcdd25c15d 100644 --- a/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationBeanPostProcessor.java +++ b/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationBeanPostProcessor.java @@ -395,6 +395,16 @@ public Object postProcessAfterInitialization(Object bean, String beanName) { return bean; } + /** + * Process the given {@code @Scheduled} method declaration on the given bean, + * attempting to distinguish {@link #processScheduledReactive(Scheduled, Method, Object) reactive} + * method from {@link #processScheduledSync(Scheduled, Method, Object) synchronous} methods. + * @param scheduled the {@code @Scheduled} annotation + * @param method the method that the annotation has been declared on + * @param bean the target bean instance + * @see #processScheduledSync(Scheduled, Method, Object) + * @see #processScheduledReactive(Scheduled, Method, Object) + */ protected void processScheduled(Scheduled scheduled, Method method, Object bean) { // Is method a Kotlin suspending function? Throws if true but reactor bridge isn't on the classpath. // Is method returning a Publisher instance? Throws if true but Reactor isn't on the classpath. @@ -406,7 +416,10 @@ protected void processScheduled(Scheduled scheduled, Method method, Object bean) } /** - * Process the given {@code @Scheduled} method declaration on the given bean. + * Process the given {@code @Scheduled} method declaration on the given bean, + * as a synchronous method. The method MUST take no arguments. Its return value + * is ignored (if any) and the scheduled invocations of the method take place + * using the underlying {@link TaskScheduler} infrastructure. * @param scheduled the {@code @Scheduled} annotation * @param method the method that the annotation has been declared on * @param bean the target bean instance @@ -547,7 +560,7 @@ protected void processScheduledSync(Scheduled scheduled, Method method, Object b * @param method the method that the annotation has been declared on, which * MUST either return a Publisher or be a Kotlin suspending function * @param bean the target bean instance - * @see #createRunnable(Object, Method) + * @see ScheduledAnnotationReactiveSupport */ protected void processScheduledReactive(Scheduled scheduled, Method method, Object bean) { try { From cfdc59bd82ee6aacd13bd7884b54895f6ff14eb2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Basl=C3=A9?= Date: Mon, 20 Feb 2023 14:39:51 +0100 Subject: [PATCH 11/19] Use ReactiveAdapterRegistry to cover more types, detect wrong param count earlier --- spring-context/spring-context.gradle | 1 + .../ScheduledAnnotationReactiveSupport.java | 72 ++++++++++--------- ...heduledAnnotationReactiveSupportTests.java | 51 ++++++++----- ...ScheduledAnnotationReactiveSupportTests.kt | 58 ++++++++++----- 4 files changed, 115 insertions(+), 67 deletions(-) diff --git a/spring-context/spring-context.gradle b/spring-context/spring-context.gradle index 2fcf0d027579..cecddeb59173 100644 --- a/spring-context/spring-context.gradle +++ b/spring-context/spring-context.gradle @@ -39,6 +39,7 @@ dependencies { testImplementation("org.awaitility:awaitility") testImplementation("jakarta.inject:jakarta.inject-tck") testImplementation("org.jetbrains.kotlinx:kotlinx-coroutines-reactor") + testImplementation("io.reactivex.rxjava3:rxjava") testRuntimeOnly("jakarta.xml.bind:jakarta.xml.bind-api") testRuntimeOnly("org.glassfish:jakarta.el") // Substitute for javax.management:jmxremote_optional:1.0.1_04 (not available on Maven Central) diff --git a/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupport.java b/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupport.java index f9b5762be34a..d6993cb097d4 100644 --- a/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupport.java +++ b/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupport.java @@ -31,6 +31,8 @@ import org.springframework.aop.support.AopUtils; import org.springframework.core.CoroutinesUtils; import org.springframework.core.KotlinDetector; +import org.springframework.core.ReactiveAdapter; +import org.springframework.core.ReactiveAdapterRegistry; import org.springframework.util.Assert; import org.springframework.util.ClassUtils; import org.springframework.util.ReflectionUtils; @@ -43,9 +45,6 @@ */ abstract class ScheduledAnnotationReactiveSupport { - static final boolean publisherPresent = ClassUtils.isPresent( - "org.reactivestreams.Publisher", ScheduledAnnotationReactiveSupport.class.getClassLoader()); - static final boolean reactorPresent = ClassUtils.isPresent( "reactor.core.publisher.Flux", ScheduledAnnotationReactiveSupport.class.getClassLoader()); @@ -54,9 +53,9 @@ abstract class ScheduledAnnotationReactiveSupport { /** * Checks that if the method is reactive, it can be scheduled. Methods are considered - * eligible for reactive scheduling if they either return an instance of - * {@code Publisher} or are a Kotlin Suspending Function. If the method isn't matching - * these criteria then this check returns {@code false}. + * eligible for reactive scheduling if they either return an instance of a type that + * can be converted to {@code Publisher} or are a Kotlin Suspending Function. + * If the method isn't matching these criteria then this check returns {@code false}. *

For reactive scheduling, Reactor MUST be present at runtime. In the case of * Kotlin, the Coroutine {@code kotlinx.coroutines.reactor} bridge MUST also be * present at runtime (in order to invoke suspending functions as a {@code Mono}). @@ -67,57 +66,64 @@ abstract class ScheduledAnnotationReactiveSupport { */ static boolean isReactive(Method method) { if (KotlinDetector.isKotlinPresent() && KotlinDetector.isSuspendingFunction(method)) { - if (coroutinesReactorPresent) { - return true; - } - else { - throw new IllegalStateException("Kotlin suspending functions may only be annotated with @Scheduled" - + "if Reactor and the Reactor-Coroutine bridge (kotlinx.coroutines.reactor) are present at runtime"); - } + //Note that suspending functions declared without args have a single Continuation parameter in reflective inspection + Assert.isTrue(method.getParameterCount() == 1,"Kotlin suspending functions may only be" + + " annotated with @Scheduled if declared without arguments"); + Assert.isTrue(coroutinesReactorPresent, "Kotlin suspending functions may only be annotated with" + + " @Scheduled if Reactor and the Reactor-Coroutine bridge (kotlinx.coroutines.reactor) are present at runtime"); + return true; } - if (!publisherPresent) { + ReactiveAdapterRegistry registry = ReactiveAdapterRegistry.getSharedInstance(); + if (!registry.hasAdapters()) { return false; } - if (!Publisher.class.isAssignableFrom(method.getReturnType())) { + Class returnType = method.getReturnType(); + ReactiveAdapter candidateAdapter = registry.getAdapter(returnType); + if (candidateAdapter == null) { return false; } - - if (reactorPresent) { - return true; - } - throw new IllegalStateException("Reactive methods returning a Publisher may only be annotated with @Scheduled" - + "if Reactor is present at runtime"); + Assert.isTrue(method.getParameterCount() == 0, "Reactive methods may only be annotated with" + + " @Scheduled if declared without arguments"); + Assert.isTrue(candidateAdapter.getDescriptor().isDeferred(), "Reactive methods may only be annotated with" + + " @Scheduled if the return type supports deferred execution"); + Assert.isTrue(reactorPresent, "Reactive methods may only be annotated with @Scheduled if Reactor is" + + " present at runtime"); + return true; } /** * Turn the invocation of the provided {@code Method} into a {@code Publisher}, - * either by reflectively invoking it and returning the resulting {@code Publisher} - * or by converting a Kotlin suspending function into a Publisher via - * {@link CoroutinesUtils}. + * either by reflectively invoking it and converting the result to a {@code Publisher} + * via {@link ReactiveAdapterRegistry} or by converting a Kotlin suspending function + * into a {@code Publisher} via {@link CoroutinesUtils}. * The {@link #isReactive(Method)} check is a precondition to calling this method. */ static Publisher getPublisherFor(Method method, Object bean) { if (KotlinDetector.isKotlinPresent() && KotlinDetector.isSuspendingFunction(method)) { - //Note that suspending functions declared without args have a single Continuation parameter in reflective inspection - Assert.isTrue(method.getParameterCount() == 1,"Kotlin suspending functions may only be annotated " - + "with @Scheduled if declared without arguments"); - return CoroutinesUtils.invokeSuspendingFunction(method, bean, (Object[]) method.getParameters()); } - Assert.isTrue(method.getParameterCount() == 0, "Reactive methods may only be annotated with " - + "@Scheduled if declared without arguments"); + ReactiveAdapterRegistry registry = ReactiveAdapterRegistry.getSharedInstance(); + Class returnType = method.getReturnType(); + ReactiveAdapter adapter = registry.getAdapter(returnType); + if (adapter == null) { + throw new IllegalArgumentException("Cannot convert the @Scheduled reactive method return type to Publisher"); + } + if (!adapter.getDescriptor().isDeferred()) { + throw new IllegalArgumentException("Cannot convert the @Scheduled reactive method return type to Publisher, " + + returnType.getSimpleName() + " is not a deferred reactive type"); + } Method invocableMethod = AopUtils.selectInvocableMethod(method, bean.getClass()); try { ReflectionUtils.makeAccessible(invocableMethod); Object r = invocableMethod.invoke(bean); - return (Publisher) r; + return adapter.toPublisher(r); } catch (InvocationTargetException ex) { - throw new IllegalArgumentException("Cannot obtain a Publisher from the @Scheduled reactive method", ex.getTargetException()); + throw new IllegalArgumentException("Cannot obtain a Publisher-convertible value from the @Scheduled reactive method", ex.getTargetException()); } catch (IllegalAccessException ex) { - throw new IllegalArgumentException("Cannot obtain a Publisher from the @Scheduled reactive method", ex); + throw new IllegalArgumentException("Cannot obtain a Publisher-convertible value from the @Scheduled reactive method", ex); } } diff --git a/spring-context/src/test/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupportTests.java b/spring-context/src/test/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupportTests.java index 0b6d56550a21..f492c570cf81 100644 --- a/spring-context/src/test/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupportTests.java +++ b/spring-context/src/test/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupportTests.java @@ -21,6 +21,8 @@ import java.util.concurrent.CompletableFuture; import java.util.concurrent.atomic.AtomicInteger; +import io.reactivex.rxjava3.core.Completable; +import io.reactivex.rxjava3.core.Flowable; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; @@ -35,6 +37,7 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; import static org.springframework.scheduling.annotation.ScheduledAnnotationReactiveSupport.getPublisherFor; +import static org.springframework.scheduling.annotation.ScheduledAnnotationReactiveSupport.isReactive; class ScheduledAnnotationReactiveSupportTests { @@ -45,21 +48,28 @@ void ensureReactor() { @ParameterizedTest @ValueSource(strings = { "mono", "flux", "monoString", "fluxString", "publisherMono", - "publisherString", "monoThrows" }) //note: monoWithParams can't be found by this test + "publisherString", "monoThrows", "flowable", "completable" }) //note: monoWithParams can't be found by this test void checkIsReactive(String method) { Method m = ReflectionUtils.findMethod(ReactiveMethods.class, method); - assertThat(ScheduledAnnotationReactiveSupport.isReactive(m)).as(m.getName()).isTrue(); + assertThat(isReactive(m)).as(m.getName()).isTrue(); } @Test void checkNotReactive() { Method string = ReflectionUtils.findMethod(ReactiveMethods.class, "oops"); - Method future = ReflectionUtils.findMethod(ReactiveMethods.class, "future"); - assertThat(ScheduledAnnotationReactiveSupport.isReactive(string)) + assertThat(isReactive(string)) .as("String-returning").isFalse(); - assertThat(ScheduledAnnotationReactiveSupport.isReactive(future)) - .as("Future-returning").isFalse(); + } + + @Test + void rejectReactiveAdaptableButNotDeferred() { + Method future = ReflectionUtils.findMethod(ReactiveMethods.class, "future"); + + assertThatIllegalArgumentException().isThrownBy( + () -> isReactive(future) + ) + .withMessage("Reactive methods may only be annotated with @Scheduled if the return type supports deferred execution"); } static class ReactiveMethods { @@ -109,6 +119,14 @@ public Mono monoThrowsIllegalAccess() throws IllegalAccessException { throw new IllegalAccessException("expected"); } + public Flowable flowable() { + return Flowable.empty(); + } + + public Completable completable() { + return Completable.complete(); + } + AtomicInteger subscription = new AtomicInteger(); public Mono trackingMono() { @@ -132,22 +150,19 @@ void init() { this.target = new ReactiveMethods(); } - @SuppressWarnings("ReactiveStreamsUnusedPublisher") @Test - void rejectWithParams() { + void isReactiveRejectsWithParams() { Method m = ReflectionUtils.findMethod(ReactiveMethods.class, "monoWithParam", String.class); - assertThat(ScheduledAnnotationReactiveSupport.isReactive(m)).as("isReactive").isTrue(); - - //static helper method - assertThatIllegalArgumentException().isThrownBy(() -> getPublisherFor(m, target)) + //isReactive rejects with context + assertThatIllegalArgumentException().isThrownBy(() -> isReactive(m)) .withMessage("Reactive methods may only be annotated with @Scheduled if declared without arguments") .withNoCause(); - //constructor of task + //constructor of task doesn't provide the context isReactive does assertThatIllegalArgumentException().isThrownBy(() -> new ScheduledAnnotationReactiveSupport.ReactiveTask( m, target, Duration.ZERO, Duration.ZERO, false)) - .withMessage("Reactive methods may only be annotated with @Scheduled if declared without arguments") + .withMessage("wrong number of arguments") .withNoCause(); } @@ -157,13 +172,13 @@ void rejectCantProducePublisher() { //static helper method assertThatIllegalArgumentException().isThrownBy(() -> getPublisherFor(m, target)) - .withMessage("Cannot obtain a Publisher from the @Scheduled reactive method") + .withMessage("Cannot obtain a Publisher-convertible value from the @Scheduled reactive method") .withCause(new IllegalStateException("expected")); //constructor of task assertThatIllegalArgumentException().isThrownBy(() -> new ScheduledAnnotationReactiveSupport.ReactiveTask( m, target, Duration.ZERO, Duration.ZERO, false)) - .withMessage("Cannot obtain a Publisher from the @Scheduled reactive method") + .withMessage("Cannot obtain a Publisher-convertible value from the @Scheduled reactive method") .withCause(new IllegalStateException("expected")); } @@ -173,13 +188,13 @@ void rejectCantAccessMethod() { //static helper method assertThatIllegalArgumentException().isThrownBy(() -> getPublisherFor(m, target)) - .withMessage("Cannot obtain a Publisher from the @Scheduled reactive method") + .withMessage("Cannot obtain a Publisher-convertible value from the @Scheduled reactive method") .withCause(new IllegalAccessException("expected")); //constructor of task assertThatIllegalArgumentException().isThrownBy(() -> new ScheduledAnnotationReactiveSupport.ReactiveTask( m, target, Duration.ZERO, Duration.ZERO, false)) - .withMessage("Cannot obtain a Publisher from the @Scheduled reactive method") + .withMessage("Cannot obtain a Publisher-convertible value from the @Scheduled reactive method") .withCause(new IllegalAccessException("expected")); } diff --git a/spring-context/src/test/kotlin/org/springframework/scheduling/annotation/KotlinScheduledAnnotationReactiveSupportTests.kt b/spring-context/src/test/kotlin/org/springframework/scheduling/annotation/KotlinScheduledAnnotationReactiveSupportTests.kt index 2fe8e0302054..7989386494be 100644 --- a/spring-context/src/test/kotlin/org/springframework/scheduling/annotation/KotlinScheduledAnnotationReactiveSupportTests.kt +++ b/spring-context/src/test/kotlin/org/springframework/scheduling/annotation/KotlinScheduledAnnotationReactiveSupportTests.kt @@ -16,10 +16,16 @@ package org.springframework.scheduling.annotation +import kotlinx.coroutines.CompletableDeferred +import kotlinx.coroutines.Deferred +import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.flowOf import org.assertj.core.api.Assertions import org.assertj.core.api.Assertions.assertThat import org.junit.jupiter.api.BeforeEach import org.junit.jupiter.api.Test +import org.junit.jupiter.params.ParameterizedTest +import org.junit.jupiter.params.provider.ValueSource import org.springframework.scheduling.annotation.ScheduledAnnotationReactiveSupport.ReactiveTask import org.springframework.scheduling.annotation.ScheduledAnnotationReactiveSupport.getPublisherFor import org.springframework.scheduling.annotation.ScheduledAnnotationReactiveSupport.isReactive @@ -33,18 +39,32 @@ class KotlinScheduledAnnotationReactiveSupportTests { @Test fun ensureReactor() { - Assertions.assertThat(ScheduledAnnotationReactiveSupport.reactorPresent).isTrue + assertThat(ScheduledAnnotationReactiveSupport.reactorPresent).isTrue } @Test fun ensureKotlinCoroutineReactorBridge() { - Assertions.assertThat(ScheduledAnnotationReactiveSupport.coroutinesReactorPresent).isTrue + assertThat(ScheduledAnnotationReactiveSupport.coroutinesReactorPresent).isTrue + } + + @ParameterizedTest + @ValueSource(strings = ["suspending", "suspendingReturns"]) + fun isReactiveSuspending(methodName: String) { + val method = ReflectionUtils.findMethod(SuspendingFunctions::class.java, methodName, Continuation::class.java)!! + assertThat(isReactive(method)).isTrue + } + + @ParameterizedTest + @ValueSource(strings = ["flow", "deferred"]) + fun isReactiveKotlinType(methodName: String) { + val method = ReflectionUtils.findMethod(SuspendingFunctions::class.java, methodName)!! + assertThat(isReactive(method)).isTrue } @Test - fun kotlinSuspendingFunctionIsNotReactive() { - val suspending = ReflectionUtils.findMethod(SuspendingFunctions::class.java, "suspending", Continuation::class.java) - Assertions.assertThat(isReactive(suspending!!)).isFalse + fun isNotReactive() { + val method = ReflectionUtils.findMethod(SuspendingFunctions::class.java, "notSuspending")!! + assertThat(isReactive(method)).isFalse } internal class SuspendingFunctions { @@ -67,6 +87,14 @@ class KotlinScheduledAnnotationReactiveSupportTests { } fun notSuspending() { } + + fun flow(): Flow { + return flowOf() + } + + fun deferred(): Deferred { + return CompletableDeferred() + } } @@ -87,20 +115,18 @@ class KotlinScheduledAnnotationReactiveSupportTests { } @Test - fun rejectWithParams() { - val m = ReflectionUtils.findMethod(SuspendingFunctions::class.java, "withParam", String::class.java, Continuation::class.java) + fun isReactiveRejectsWithParams() { + val m = ReflectionUtils.findMethod(SuspendingFunctions::class.java, "withParam", String::class.java, Continuation::class.java)!! - //static helper method - Assertions.assertThatIllegalArgumentException().isThrownBy { getPublisherFor(m!!, target!!) } + //isReactive rejects with some context + Assertions.assertThatIllegalArgumentException().isThrownBy { isReactive(m) } .withMessage("Kotlin suspending functions may only be annotated with @Scheduled if declared without arguments") .withNoCause() - //constructor of task - Assertions.assertThatIllegalArgumentException().isThrownBy { - ReactiveTask(m!!, target!!, Duration.ZERO, Duration.ZERO, false) + //constructor of task doesn't reject + Assertions.assertThatNoException().isThrownBy { + ReactiveTask(m, target!!, Duration.ZERO, Duration.ZERO, false) } - .withMessage("Kotlin suspending functions may only be annotated with @Scheduled if declared without arguments") - .withNoCause() } @Test @@ -109,14 +135,14 @@ class KotlinScheduledAnnotationReactiveSupportTests { //static helper method Assertions.assertThatIllegalArgumentException().isThrownBy { getPublisherFor(m!!, target!!) } - .withMessage("Kotlin suspending functions may only be annotated with @Scheduled if declared without arguments") + .withMessage("Cannot convert the @Scheduled reactive method return type to Publisher") .withNoCause() //constructor of task Assertions.assertThatIllegalArgumentException().isThrownBy { ReactiveTask(m!!, target!!, Duration.ZERO, Duration.ZERO, false) } - .withMessage("Kotlin suspending functions may only be annotated with @Scheduled if declared without arguments") + .withMessage("Cannot convert the @Scheduled reactive method return type to Publisher") .withNoCause() } From 490557676bbba6f9ffe62a7b700569c50c973374 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Basl=C3=A9?= Date: Mon, 20 Feb 2023 14:57:03 +0100 Subject: [PATCH 12/19] Polish reference guide + annotation javadoc --- .../ROOT/pages/integration/scheduling.adoc | 44 ++++++++++++++++--- .../scheduling/annotation/Scheduled.java | 8 ++-- 2 files changed, 43 insertions(+), 9 deletions(-) diff --git a/framework-docs/modules/ROOT/pages/integration/scheduling.adoc b/framework-docs/modules/ROOT/pages/integration/scheduling.adoc index 4da17e246348..2dc0b89c4ef3 100644 --- a/framework-docs/modules/ROOT/pages/integration/scheduling.adoc +++ b/framework-docs/modules/ROOT/pages/integration/scheduling.adoc @@ -396,11 +396,11 @@ container and once through the `@Configurable` aspect), with the consequence of [[scheduling-annotation-support-scheduled-reactive]] === The `@Scheduled` annotation on Reactive methods or Kotlin suspending functions -As of Spring Framework 6.1, `@Scheduled` methods are also supported on two types -of asynchronous methods: +As of Spring Framework 6.1, `@Scheduled` methods are also supported on several types +of reactive methods: - - Reactive methods, i.e. methods with a `Publisher` return type (or any concrete -implementation of `Publisher`) like in the following example: + - methods with a `Publisher` return type (or any concrete implementation of `Publisher`) +like in the following example: [source,java,indent=0,subs="verbatim,quotes"] ---- @@ -410,6 +410,26 @@ implementation of `Publisher`) like in the following example: } ---- + - methods with a return type that can be adapted to `Publisher` via the shared instance +of the `ReactiveAdapterRegistry`, provided the type supports _deferred subscription_ like +in the following example: + +[source,java,indent=0,subs="verbatim,quotes"] +---- + @Scheduled(fixedDelay = 500) + public Single rxjavaNonPublisher() { + return Single.just("example"); + } +---- + +[NOTE] +==== +The `CompletableFuture` class is an example of a type that can typically be adapted +to `Publisher` but doesn't support deferred subscription. Its `ReactiveAdapter` in the +registry denotes that by having the `getDescriptor().isDeferred()` method return `false`. +==== + + - Kotlin suspending functions, like in the following example: [source,kotlin,indent=0,subs="verbatim,quotes"] @@ -420,8 +440,20 @@ implementation of `Publisher`) like in the following example: } ---- -Both types of methods must be declared without any arguments. Additionally, the `cron` -configuration elements are not supported for these two cases. + - methods that return a Kotlin `Flow` or `Deferred` instance, like in the following example: + +[source,kotlin,indent=0,subs="verbatim,quotes"] +---- + @Scheduled(fixedDelay = 500) + fun something(): Flow { + flow { + // do something asynchronous + } + } +---- + +All these types of methods must be declared without any arguments. Additionally, the `cron` +configuration elements are not supported for these cases. Note that the `TaskExecutor`/`TaskScheduler` infrastructure is not used and the scheduling is instead performed by the Reactor library, which must be present at runtime. diff --git a/spring-context/src/main/java/org/springframework/scheduling/annotation/Scheduled.java b/spring-context/src/main/java/org/springframework/scheduling/annotation/Scheduled.java index e589589a886d..92840526d8bc 100644 --- a/spring-context/src/main/java/org/springframework/scheduling/annotation/Scheduled.java +++ b/spring-context/src/main/java/org/springframework/scheduling/annotation/Scheduled.java @@ -36,9 +36,11 @@ * a {@code void} return type; if not, the returned value will be ignored * when called through the scheduler. * - *

Methods that return a reactive {@code Publisher} are supported provided the - * Reactor library is present at runtime: it is used to implement the scheduling by - * repeatedly subscribing to the returned Publisher, which is only produced once. + *

Methods that return a reactive {@code Publisher} or a type which can be adapted + * to {@code Publisher} by the default {@code ReactiveAdapterRegistry} are supported + * provided the Reactor library is present at runtime. Reactor is used to implement + * the scheduling by repeatedly subscribing to the returned Publisher, which is only + * produced once. * The cron configuration is not supported for this type of method. Values emitted by * the publisher are ignored and discarded. Errors are logged at WARN level, which * doesn't prevent further iterations. From 37c1cfae09588ef78700033c7b6e9759ef302057 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Basl=C3=A9?= Date: Thu, 20 Apr 2023 10:57:46 +0200 Subject: [PATCH 13/19] polish: remove FIXME --- .../annotation/ScheduledAnnotationReactiveSupport.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupport.java b/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupport.java index d6993cb097d4..1dd69568068a 100644 --- a/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupport.java +++ b/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupport.java @@ -41,7 +41,7 @@ * Helper class for @{@link ScheduledAnnotationBeanPostProcessor} to support reactive cases * without a dependency on optional classes. * @author Simon Baslé - * @since 6.1.0 //FIXME + * @since 6.1.0 */ abstract class ScheduledAnnotationReactiveSupport { From ffdbc2711fd9afb81c02011fa80de371df2ead4c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Basl=C3=A9?= Date: Mon, 24 Apr 2023 17:26:29 +0200 Subject: [PATCH 14/19] polish indentation --- .../ScheduledAnnotationReactiveSupportTests.java | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/spring-context/src/test/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupportTests.java b/spring-context/src/test/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupportTests.java index f492c570cf81..77c6e8450730 100644 --- a/spring-context/src/test/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupportTests.java +++ b/spring-context/src/test/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupportTests.java @@ -66,10 +66,8 @@ void checkNotReactive() { void rejectReactiveAdaptableButNotDeferred() { Method future = ReflectionUtils.findMethod(ReactiveMethods.class, "future"); - assertThatIllegalArgumentException().isThrownBy( - () -> isReactive(future) - ) - .withMessage("Reactive methods may only be annotated with @Scheduled if the return type supports deferred execution"); + assertThatIllegalArgumentException().isThrownBy(() -> isReactive(future)) + .withMessage("Reactive methods may only be annotated with @Scheduled if the return type supports deferred execution"); } static class ReactiveMethods { @@ -204,7 +202,7 @@ void hasCheckpointToString() { final ScheduledAnnotationReactiveSupport.ReactiveTask reactiveTask = new ScheduledAnnotationReactiveSupport.ReactiveTask( m, target, Duration.ZERO, Duration.ZERO, false); - assertThat(reactiveTask).hasToString("@Scheduled 'mono()' in bean 'org.springframework.scheduling.annotation.ScheduledAnnotationReactiveSupportTests$ReactiveMethods'"); + assertThat(reactiveTask).hasToString("@Scheduled 'mono()' in bean 'org.springframework.scheduling.annotation.ScheduledAnnotationReactiveSupportTests$ReactiveMethods'"); } @Test From a413f4f14708e3dad928b510d43d1621c73a89b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Basl=C3=A9?= Date: Tue, 25 Apr 2023 15:39:49 +0200 Subject: [PATCH 15/19] Pivot to use the existing Runnable-based infrastructure --- .../ROOT/pages/integration/scheduling.adoc | 21 +- spring-context/spring-context.gradle | 1 + .../annotation/EnableScheduling.java | 5 - .../scheduling/annotation/Scheduled.java | 19 +- .../ScheduledAnnotationBeanPostProcessor.java | 184 +++++------------- .../ScheduledAnnotationReactiveSupport.java | 166 ++++++++-------- ...heduledAnnotationReactiveSupportTests.java | 183 ++++------------- 7 files changed, 187 insertions(+), 392 deletions(-) diff --git a/framework-docs/modules/ROOT/pages/integration/scheduling.adoc b/framework-docs/modules/ROOT/pages/integration/scheduling.adoc index 2dc0b89c4ef3..a30aa5da4601 100644 --- a/framework-docs/modules/ROOT/pages/integration/scheduling.adoc +++ b/framework-docs/modules/ROOT/pages/integration/scheduling.adoc @@ -452,17 +452,13 @@ registry denotes that by having the `getDescriptor().isDeferred()` method return } ---- -All these types of methods must be declared without any arguments. Additionally, the `cron` -configuration elements are not supported for these cases. - -Note that the `TaskExecutor`/`TaskScheduler` infrastructure is not used and the scheduling -is instead performed by the Reactor library, which must be present at runtime. -In the case of Kotlin suspending functions the `kotlinx.coroutines.reactor` bridge must -also be present. +All these types of methods must be declared without any arguments. In the case of Kotlin +suspending functions the `kotlinx.coroutines.reactor` bridge must also be present to allow +the framework to invoke a suspending function as a `Publisher`. The Spring Framework will obtain a `Publisher` out of the annotated method once and will -create a timed `Flux` which regularly subscribes to said `Publisher`. These inner regular -subscriptions happen according to the `fixedDelay`/`fixedRate` configuration. +schedule a `Runnable` in which it subscribes to said `Publisher`. These inner regular +subscriptions happen according to the `cron`/fixedDelay`/`fixedRate` configuration. If the `Publisher` emits `onNext` signal(s), these are ignored and discarded (the same way return values from synchronous `@Scheduled` methods are ignored). @@ -502,8 +498,11 @@ seconds: [NOTE] ==== -The Spring Framework subscribes once to the underlying scheduling `Flux`, which can be -cancelled by destroying the annotated bean or closing the application context. +When destroying the annotated bean or closing the application context Spring Framework cancels +scheduled tasks, which includes the next scheduled subscription to the `Publisher`. +However, there is no tracking of individual subscriptions: once a subscription has started +it can't be cancelled. For that reason, it is a pre-requisite that the `Publisher` is finite +in addition to supporting multiple subsequent subscriptions. ==== diff --git a/spring-context/spring-context.gradle b/spring-context/spring-context.gradle index cecddeb59173..333ec1dd2ad8 100644 --- a/spring-context/spring-context.gradle +++ b/spring-context/spring-context.gradle @@ -38,6 +38,7 @@ dependencies { testImplementation("org.apache.commons:commons-pool2") testImplementation("org.awaitility:awaitility") testImplementation("jakarta.inject:jakarta.inject-tck") + testImplementation("org.jetbrains.kotlinx:kotlinx-coroutines-core") testImplementation("org.jetbrains.kotlinx:kotlinx-coroutines-reactor") testImplementation("io.reactivex.rxjava3:rxjava") testRuntimeOnly("jakarta.xml.bind:jakarta.xml.bind-api") diff --git a/spring-context/src/main/java/org/springframework/scheduling/annotation/EnableScheduling.java b/spring-context/src/main/java/org/springframework/scheduling/annotation/EnableScheduling.java index 5fd0f049a34b..0d49e240d5df 100644 --- a/spring-context/src/main/java/org/springframework/scheduling/annotation/EnableScheduling.java +++ b/spring-context/src/main/java/org/springframework/scheduling/annotation/EnableScheduling.java @@ -188,11 +188,6 @@ * application context and any separate {@code DispatcherServlet} application contexts, * if you need to apply its behavior at multiple levels. * - *

Note: {@code @EnableScheduling} and {@code @Scheduled} support of reactive methods - * and Kotlin suspending functions uses Reactor infrastructure instead of the - * {@code ScheduledTaskRegistrar} infrastructure, so previously discussed task-related - * aspects like {@code SchedulingConfigurer} don't apply to these two cases. - * * @author Chris Beams * @author Juergen Hoeller * @since 3.1 diff --git a/spring-context/src/main/java/org/springframework/scheduling/annotation/Scheduled.java b/spring-context/src/main/java/org/springframework/scheduling/annotation/Scheduled.java index 92840526d8bc..6e9ddc579b2f 100644 --- a/spring-context/src/main/java/org/springframework/scheduling/annotation/Scheduled.java +++ b/spring-context/src/main/java/org/springframework/scheduling/annotation/Scheduled.java @@ -37,17 +37,18 @@ * when called through the scheduler. * *

Methods that return a reactive {@code Publisher} or a type which can be adapted - * to {@code Publisher} by the default {@code ReactiveAdapterRegistry} are supported - * provided the Reactor library is present at runtime. Reactor is used to implement - * the scheduling by repeatedly subscribing to the returned Publisher, which is only - * produced once. - * The cron configuration is not supported for this type of method. Values emitted by - * the publisher are ignored and discarded. Errors are logged at WARN level, which - * doesn't prevent further iterations. + * to {@code Publisher} by the default {@code ReactiveAdapterRegistry} are supported. + * The {@code Publisher} MUST be finite and MUST support multiple subsequent subscriptions + * (i.e. be cold). + * The returned Publisher is only produced once, and the scheduling infrastructure + * then periodically {@code subscribe()} to it according to configuration. + * Values emitted by the publisher are ignored. Errors are logged at WARN level, which + * doesn't prevent further iterations. If a {@code fixed delay} is configured, the + * subscription is blocked upon in order to respect the fixed delay semantics. * - *

Kotlin suspending functions are also supported, provided the coroutine-Reactor + *

Kotlin suspending functions are also supported, provided the coroutine-reactor * bridge ({@code kotlinx.coroutine.reactor}) is present at runtime. This bridge is - * used to adapt the suspending function into a Reactor {@code Mono} which is treated + * used to adapt the suspending function into a {@code Publisher} which is treated * the same way as in the reactive method case (see above). * *

Processing of {@code @Scheduled} annotations is performed by diff --git a/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationBeanPostProcessor.java b/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationBeanPostProcessor.java index f3bcdd25c15d..4a894bcce02c 100644 --- a/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationBeanPostProcessor.java +++ b/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationBeanPostProcessor.java @@ -143,7 +143,6 @@ public class ScheduledAnnotationBeanPostProcessor private final Set> nonAnnotatedClasses = Collections.newSetFromMap(new ConcurrentHashMap<>(64)); private final Map> scheduledTasks = new IdentityHashMap<>(16); - private final Map> scheduledReactiveTasks = new IdentityHashMap<>(16); /** @@ -318,15 +317,6 @@ private void finishRegistration() { } this.registrar.afterPropertiesSet(); - // Start the reactive tasks (we synchronize on the common scheduledTasks on purpose) - synchronized (this.scheduledTasks) { - for (Set reactiveTasks : this.scheduledReactiveTasks.values()) { - for (ScheduledAnnotationReactiveSupport.ReactiveTask reactiveTask : reactiveTasks) { - reactiveTask.subscribe(); - } - } - } - } private T resolveSchedulerBean(BeanFactory beanFactory, Class schedulerType, boolean byName) { @@ -397,37 +387,32 @@ public Object postProcessAfterInitialization(Object bean, String beanName) { /** * Process the given {@code @Scheduled} method declaration on the given bean, - * attempting to distinguish {@link #processScheduledReactive(Scheduled, Method, Object) reactive} - * method from {@link #processScheduledSync(Scheduled, Method, Object) synchronous} methods. + * attempting to distinguish {@link #processScheduledAsync(Scheduled, Method, Object) reactive} + * methods from {@link #processScheduledSync(Scheduled, Method, Object) synchronous} methods. * @param scheduled the {@code @Scheduled} annotation * @param method the method that the annotation has been declared on * @param bean the target bean instance * @see #processScheduledSync(Scheduled, Method, Object) - * @see #processScheduledReactive(Scheduled, Method, Object) + * @see #processScheduledAsync(Scheduled, Method, Object) */ protected void processScheduled(Scheduled scheduled, Method method, Object bean) { // Is method a Kotlin suspending function? Throws if true but reactor bridge isn't on the classpath. - // Is method returning a Publisher instance? Throws if true but Reactor isn't on the classpath. + // Is method returning a reactive type? Throws if true, but it isn't a deferred Publisher type. if (ScheduledAnnotationReactiveSupport.isReactive(method)) { - processScheduledReactive(scheduled, method, bean); + processScheduledAsync(scheduled, method, bean); return; } processScheduledSync(scheduled, method, bean); } /** - * Process the given {@code @Scheduled} method declaration on the given bean, - * as a synchronous method. The method MUST take no arguments. Its return value - * is ignored (if any) and the scheduled invocations of the method take place - * using the underlying {@link TaskScheduler} infrastructure. - * @param scheduled the {@code @Scheduled} annotation - * @param method the method that the annotation has been declared on - * @param bean the target bean instance - * @see #createRunnable(Object, Method) + * Parse the {@code Scheduled} annotation and schedule the provided {@code Runnable} + * accordingly. The Runnable can represent either a synchronous method invocation + * (see {@link #processScheduledSync(Scheduled, Method, Object)}) or an asynchronous + * one (see {@link #processScheduledAsync(Scheduled, Method, Object)}). */ - protected void processScheduledSync(Scheduled scheduled, Method method, Object bean) { + protected void processScheduledTask(Scheduled scheduled, Runnable runnable, Method method, Object bean) { try { - Runnable runnable = createRunnable(bean, method); boolean processedSchedule = false; String errorMessage = "Exactly one of the 'cron', 'fixedDelay(String)', or 'fixedRate(String)' attributes is required"; @@ -550,118 +535,51 @@ protected void processScheduledSync(Scheduled scheduled, Method method, Object b } } + /** + * Process the given {@code @Scheduled} method declaration on the given bean, + * as a synchronous method. The method MUST take no arguments. Its return value + * is ignored (if any) and the scheduled invocations of the method take place + * using the underlying {@link TaskScheduler} infrastructure. + * @param scheduled the {@code @Scheduled} annotation + * @param method the method that the annotation has been declared on + * @param bean the target bean instance + * @see #createRunnable(Object, Method) + */ + protected void processScheduledSync(Scheduled scheduled, Method method, Object bean) { + Runnable task; + try { + task = createRunnable(bean, method); + } + catch (IllegalArgumentException ex) { + throw new IllegalStateException("Could not create recurring task for @Scheduled method '" + method.getName() + "': " + ex.getMessage()); + } + processScheduledTask(scheduled, task, method, bean); + } + /** * Process the given {@code @Scheduled} bean method declaration which returns * a {@code Publisher}, or the given Kotlin suspending function converted to a - * Publisher. The publisher is then repeatedly subscribed to, according to the - * fixedDelay/fixedRate configuration. Cron configuration isn't supported,nor - * is non-Publisher return types (even if a {@code ReactiveAdapter} is registered). + * Publisher. A {@code Runnable} which subscribes to that publisher is then repeatedly + * scheduled according to the annotation configuration. + *

Note that for fixed delay configuration, the subscription is turned into a blocking + * call instead. Types for which a {@code ReactiveAdapter} is registered but which cannot + * be deferred (i.e. not a {@code Publisher}) are not supported. * @param scheduled the {@code @Scheduled} annotation * @param method the method that the annotation has been declared on, which - * MUST either return a Publisher or be a Kotlin suspending function + * MUST either return a Publisher-adaptable type or be a Kotlin suspending function * @param bean the target bean instance * @see ScheduledAnnotationReactiveSupport */ - protected void processScheduledReactive(Scheduled scheduled, Method method, Object bean) { + protected void processScheduledAsync(Scheduled scheduled, Method method, Object bean) { + Runnable task; try { - boolean processedSchedule = false; - String errorMessage = - "Exactly one of the 'fixedDelay(String)' or 'fixedRate(String)' attributes is required"; - - Set reactiveTasks = new LinkedHashSet<>(4); - - // Determine initial delay - Duration initialDelay = toDuration(scheduled.initialDelay(), scheduled.timeUnit()); - String initialDelayString = scheduled.initialDelayString(); - if (StringUtils.hasText(initialDelayString)) { - Assert.isTrue(initialDelay.isNegative(), "Specify 'initialDelay' or 'initialDelayString', not both"); - if (this.embeddedValueResolver != null) { - initialDelayString = this.embeddedValueResolver.resolveStringValue(initialDelayString); - } - if (StringUtils.hasLength(initialDelayString)) { - try { - initialDelay = toDuration(initialDelayString, scheduled.timeUnit()); - } - catch (RuntimeException ex) { - throw new IllegalArgumentException( - "Invalid initialDelayString value \"" + initialDelayString + "\" - cannot parse into long"); - } - } - } - - // Reject cron expression - Assert.state(!StringUtils.hasText(scheduled.cron()), "'cron' not supported for reactive @Scheduled"); - - // At this point we don't need to differentiate between initial delay set or not anymore - if (initialDelay.isNegative()) { - initialDelay = Duration.ZERO; - } - - // Check fixed delay - Duration fixedDelay = toDuration(scheduled.fixedDelay(), scheduled.timeUnit()); - if (!fixedDelay.isNegative()) { - processedSchedule = true; - reactiveTasks.add(new ScheduledAnnotationReactiveSupport.ReactiveTask(method, bean, initialDelay, fixedDelay, false)); - } - - String fixedDelayString = scheduled.fixedDelayString(); - if (StringUtils.hasText(fixedDelayString)) { - if (this.embeddedValueResolver != null) { - fixedDelayString = this.embeddedValueResolver.resolveStringValue(fixedDelayString); - } - if (StringUtils.hasLength(fixedDelayString)) { - Assert.isTrue(!processedSchedule, errorMessage); - processedSchedule = true; - try { - fixedDelay = toDuration(fixedDelayString, scheduled.timeUnit()); - } - catch (RuntimeException ex) { - throw new IllegalArgumentException( - "Invalid fixedDelayString value \"" + fixedDelayString + "\" - cannot parse into long"); - } - reactiveTasks.add(new ScheduledAnnotationReactiveSupport.ReactiveTask(method, bean, initialDelay, fixedDelay, false)); - } - } - - // Check fixed rate - Duration fixedRate = toDuration(scheduled.fixedRate(), scheduled.timeUnit()); - if (!fixedRate.isNegative()) { - Assert.isTrue(!processedSchedule, errorMessage); - processedSchedule = true; - reactiveTasks.add(new ScheduledAnnotationReactiveSupport.ReactiveTask(method, bean, initialDelay, fixedRate, true)); - } - String fixedRateString = scheduled.fixedRateString(); - if (StringUtils.hasText(fixedRateString)) { - if (this.embeddedValueResolver != null) { - fixedRateString = this.embeddedValueResolver.resolveStringValue(fixedRateString); - } - if (StringUtils.hasLength(fixedRateString)) { - Assert.isTrue(!processedSchedule, errorMessage); - processedSchedule = true; - try { - fixedRate = toDuration(fixedRateString, scheduled.timeUnit()); - } - catch (RuntimeException ex) { - throw new IllegalArgumentException( - "Invalid fixedRateString value \"" + fixedRateString + "\" - cannot parse into long"); - } - reactiveTasks.add(new ScheduledAnnotationReactiveSupport.ReactiveTask(method, bean, initialDelay, fixedRate, true)); - } - } - - // Check whether we had any attribute set - Assert.isTrue(processedSchedule, errorMessage); - - // Finally register the scheduled tasks (we synchronize on scheduledTasks on purpose) - synchronized (this.scheduledTasks) { - Set subscriptionTasks = this.scheduledReactiveTasks.computeIfAbsent(bean, key -> new LinkedHashSet<>(4)); - subscriptionTasks.addAll(reactiveTasks); - } + boolean isFixedDelaySpecialCase = scheduled.fixedDelay() > 0 || StringUtils.hasText(scheduled.fixedDelayString()); + task = ScheduledAnnotationReactiveSupport.createSubscriptionRunnable(method, bean, isFixedDelaySpecialCase); } catch (IllegalArgumentException ex) { - throw new IllegalStateException( - "Encountered invalid reactive @Scheduled method '" + method.getName() + "': " + ex.getMessage(), ex); + throw new IllegalStateException("Could not create recurring task for @Scheduled method '" + method.getName() + "': " + ex.getMessage()); } + processScheduledTask(scheduled, task, method, bean); } /** @@ -721,27 +639,20 @@ public Set getScheduledTasks() { @Override public void postProcessBeforeDestruction(Object bean, String beanName) { Set tasks; - Set reactiveTasks; synchronized (this.scheduledTasks) { tasks = this.scheduledTasks.remove(bean); - reactiveTasks = this.scheduledReactiveTasks.remove(bean); } if (tasks != null) { for (ScheduledTask task : tasks) { task.cancel(); } } - if (reactiveTasks != null) { - for (ScheduledAnnotationReactiveSupport.ReactiveTask task : reactiveTasks) { - task.cancel(); - } - } } @Override public boolean requiresDestruction(Object bean) { synchronized (this.scheduledTasks) { - return this.scheduledTasks.containsKey(bean) || this.scheduledReactiveTasks.containsKey(bean); + return this.scheduledTasks.containsKey(bean); } } @@ -755,13 +666,6 @@ public void destroy() { } } this.scheduledTasks.clear(); - Collection> allReactiveTasks = this.scheduledReactiveTasks.values(); - for (Set tasks : allReactiveTasks) { - for (ScheduledAnnotationReactiveSupport.ReactiveTask task : tasks) { - task.cancel(); - } - } - this.scheduledReactiveTasks.clear(); } this.registrar.destroy(); } diff --git a/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupport.java b/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupport.java index 1dd69568068a..47fb6a0278c7 100644 --- a/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupport.java +++ b/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupport.java @@ -18,15 +18,14 @@ import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; -import java.time.Duration; +import java.util.concurrent.CountDownLatch; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.reactivestreams.Publisher; -import reactor.core.Disposable; -import reactor.core.Disposables; +import org.reactivestreams.Subscriber; +import org.reactivestreams.Subscription; import reactor.core.publisher.Flux; -import reactor.core.publisher.Mono; import org.springframework.aop.support.AopUtils; import org.springframework.core.CoroutinesUtils; @@ -51,14 +50,16 @@ abstract class ScheduledAnnotationReactiveSupport { static final boolean coroutinesReactorPresent = ClassUtils.isPresent( "kotlinx.coroutines.reactor.MonoKt", ScheduledAnnotationReactiveSupport.class.getClassLoader()); + private static final Log LOGGER = LogFactory.getLog(ScheduledAnnotationReactiveSupport.class); + /** * Checks that if the method is reactive, it can be scheduled. Methods are considered * eligible for reactive scheduling if they either return an instance of a type that * can be converted to {@code Publisher} or are a Kotlin Suspending Function. * If the method isn't matching these criteria then this check returns {@code false}. - *

For reactive scheduling, Reactor MUST be present at runtime. In the case of - * Kotlin, the Coroutine {@code kotlinx.coroutines.reactor} bridge MUST also be - * present at runtime (in order to invoke suspending functions as a {@code Mono}). + *

For scheduling of Kotlin Suspending Functions, the Coroutine-Reactor bridge + * {@code kotlinx.coroutines.reactor} MUST be present at runtime (in order to invoke + * suspending functions as a {@code Publisher}). * Provided that is the case, this method returns {@code true}. Otherwise, it throws * an {@code IllegalStateException}. * @throws IllegalStateException if the method is reactive but Reactor and/or the @@ -70,7 +71,7 @@ static boolean isReactive(Method method) { Assert.isTrue(method.getParameterCount() == 1,"Kotlin suspending functions may only be" + " annotated with @Scheduled if declared without arguments"); Assert.isTrue(coroutinesReactorPresent, "Kotlin suspending functions may only be annotated with" - + " @Scheduled if Reactor and the Reactor-Coroutine bridge (kotlinx.coroutines.reactor) are present at runtime"); + + " @Scheduled if the Coroutine-Reactor bridge (kotlinx.coroutines.reactive) is present at runtime"); return true; } ReactiveAdapterRegistry registry = ReactiveAdapterRegistry.getSharedInstance(); @@ -86,8 +87,6 @@ static boolean isReactive(Method method) { + " @Scheduled if declared without arguments"); Assert.isTrue(candidateAdapter.getDescriptor().isDeferred(), "Reactive methods may only be annotated with" + " @Scheduled if the return type supports deferred execution"); - Assert.isTrue(reactorPresent, "Reactive methods may only be annotated with @Scheduled if Reactor is" - + " present at runtime"); return true; } @@ -97,6 +96,8 @@ static boolean isReactive(Method method) { * via {@link ReactiveAdapterRegistry} or by converting a Kotlin suspending function * into a {@code Publisher} via {@link CoroutinesUtils}. * The {@link #isReactive(Method)} check is a precondition to calling this method. + * If Reactor is present at runtime, the Publisher is additionally converted to a {@code Flux} + * with a checkpoint String, allowing for better debugging. */ static Publisher getPublisherFor(Method method, Object bean) { if (KotlinDetector.isKotlinPresent() && KotlinDetector.isSuspendingFunction(method)) { @@ -117,7 +118,17 @@ static Publisher getPublisherFor(Method method, Object bean) { try { ReflectionUtils.makeAccessible(invocableMethod); Object r = invocableMethod.invoke(bean); - return adapter.toPublisher(r); + + Publisher publisher = adapter.toPublisher(r); + //if Reactor is on the classpath, we could benefit from having a checkpoint for debuggability + if (reactorPresent) { + final String checkpoint = "@Scheduled '"+ method.getName() + "()' in bean '" + + method.getDeclaringClass().getName() + "'"; + return Flux.from(publisher).checkpoint(checkpoint); + } + else { + return publisher; + } } catch (InvocationTargetException ex) { throw new IllegalArgumentException("Cannot obtain a Publisher-convertible value from the @Scheduled reactive method", ex.getTargetException()); @@ -128,88 +139,71 @@ static Publisher getPublisherFor(Method method, Object bean) { } /** - * Encapsulates the logic of {@code @Scheduled} on reactive types, using Reactor. - * The {@link ScheduledAnnotationReactiveSupport#isReactive(Method)} check is a - * precondition to instantiating this class. + * Create a {@link Runnable} for the Scheduled infrastructure, allowing for scheduled + * subscription to the publisher produced by a reactive method. + *

Note that the reactive method is invoked once, but the resulting {@code Publisher} + * is subscribed to repeatedly, once per each invocation of the {@code Runnable}. + *

In the case of a {@code fixed delay} configuration, the subscription inside the + * Runnable is turned into a blocking call in order to maintain fixedDelay semantics + * (i.e. the task blocks until completion of the Publisher, then the delay is applied + * until next iteration). */ - static class ReactiveTask { - - private final Publisher publisher; - private final Duration initialDelay; - private final Duration otherDelay; - private final boolean isFixedRate; - protected final String checkpoint; - private final Disposable.Swap disposable; - - private final Log logger = LogFactory.getLog(getClass()); - - - protected ReactiveTask(Method method, Object bean, Duration initialDelay, Duration otherDelay, boolean isFixedRate) { - this.publisher = getPublisherFor(method, bean); - - this.initialDelay = initialDelay; - this.otherDelay = otherDelay; - this.isFixedRate = isFixedRate; - - this.disposable = Disposables.swap(); - this.checkpoint = "@Scheduled '"+ method.getName() + "()' in bean '" - + method.getDeclaringClass().getName() + "'"; + static Runnable createSubscriptionRunnable(Method method, Object targetBean, boolean isFixedDelaySpecialCase) { + final Publisher publisher = getPublisherFor(method, targetBean); + if (isFixedDelaySpecialCase) { + return () -> { + final CountDownLatch latch = new CountDownLatch(1); + publisher.subscribe(new Subscriber() { + @Override + public void onSubscribe(Subscription s) { + s.request(Integer.MAX_VALUE); + } + + @Override + public void onNext(Object o) { + // NO-OP + } + + @Override + public void onError(Throwable e) { + LOGGER.warn("Unexpected error occurred in scheduled reactive task", e); + latch.countDown(); + } + + @Override + public void onComplete() { + latch.countDown(); + } + }); + try { + latch.await(); + } + catch (InterruptedException e) { + throw new RuntimeException(e); + } + }; } - - private Mono safeExecutionMono() { - Mono executionMono; - if (this.publisher instanceof Mono) { - executionMono = Mono.from(this.publisher).then(); + return () -> publisher.subscribe(new Subscriber() { + @Override + public void onSubscribe(Subscription s) { + s.request(Integer.MAX_VALUE); } - else { - executionMono = Flux.from(this.publisher).then(); - } - if (logger.isWarnEnabled()) { - executionMono = executionMono.doOnError(ex -> logger.warn( - "Ignored error in publisher from " + this.checkpoint, ex)); - } - executionMono = executionMono.onErrorComplete(); - return executionMono; - } - public void subscribe() { - if (this.disposable.isDisposed()) { - return; + @Override + public void onNext(Object o) { + // NO-OP } - final Mono executionMono = safeExecutionMono(); - Flux scheduledFlux; - if (this.isFixedRate) { - scheduledFlux = Flux.interval(this.initialDelay, this.otherDelay) - .flatMap(it -> executionMono); - } - else { - scheduledFlux = Mono.delay(this.otherDelay).then(executionMono).repeat(); - if (!this.initialDelay.isZero()) { - scheduledFlux = Flux.concat( - Mono.delay(this.initialDelay).then(executionMono), - scheduledFlux - ); - } - else { - scheduledFlux = Flux.concat(executionMono, scheduledFlux); - } + @Override + public void onError(Throwable e) { + LOGGER.warn("Unexpected error occurred in scheduled reactive task", e); } - // Subscribe and ensure that errors can be traced back to the @Scheduled via a checkpoint - if (this.disposable.isDisposed()) { - return; - } - this.disposable.update(scheduledFlux.checkpoint(this.checkpoint) - .subscribe(it -> {}, ex -> ReactiveTask.this.logger.error("Unexpected error occurred in scheduled reactive task", ex))); - } - - public void cancel() { - this.disposable.dispose(); - } - @Override - public String toString() { - return this.checkpoint; - } + @Override + public void onComplete() { + // NO-OP + } + }); } + } diff --git a/spring-context/src/test/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupportTests.java b/spring-context/src/test/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupportTests.java index 77c6e8450730..af3f6ee83eb2 100644 --- a/spring-context/src/test/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupportTests.java +++ b/spring-context/src/test/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupportTests.java @@ -17,14 +17,11 @@ package org.springframework.scheduling.annotation; import java.lang.reflect.Method; -import java.time.Duration; import java.util.concurrent.CompletableFuture; import java.util.concurrent.atomic.AtomicInteger; import io.reactivex.rxjava3.core.Completable; import io.reactivex.rxjava3.core.Flowable; -import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.ValueSource; @@ -70,6 +67,48 @@ void rejectReactiveAdaptableButNotDeferred() { .withMessage("Reactive methods may only be annotated with @Scheduled if the return type supports deferred execution"); } + @Test + void isReactiveRejectsWithParams() { + Method m = ReflectionUtils.findMethod(ReactiveMethods.class, "monoWithParam", String.class); + + //isReactive rejects with context + assertThatIllegalArgumentException().isThrownBy(() -> isReactive(m)) + .withMessage("Reactive methods may only be annotated with @Scheduled if declared without arguments") + .withNoCause(); + } + + @Test + void rejectCantProducePublisher() { + ReactiveMethods target = new ReactiveMethods(); + Method m = ReflectionUtils.findMethod(ReactiveMethods.class, "monoThrows"); + + //static helper method + assertThatIllegalArgumentException().isThrownBy(() -> getPublisherFor(m, target)) + .withMessage("Cannot obtain a Publisher-convertible value from the @Scheduled reactive method") + .withCause(new IllegalStateException("expected")); + } + + @Test + void rejectCantAccessMethod() { + ReactiveMethods target = new ReactiveMethods(); + Method m = ReflectionUtils.findMethod(ReactiveMethods.class, "monoThrowsIllegalAccess"); + + //static helper method + assertThatIllegalArgumentException().isThrownBy(() -> getPublisherFor(m, target)) + .withMessage("Cannot obtain a Publisher-convertible value from the @Scheduled reactive method") + .withCause(new IllegalAccessException("expected")); + } + + //FIXME find a way to test the case with fixedDelay effectively turning into a fixedRate ? + + //FIXME test createCheckpointPublisherFor uses Reactor and checkpoint operator + + @Test + void hasCheckpointToString() { + //FIXME test checkpointing + assertThat("FIXME").isEqualTo("@Scheduled 'mono()' in bean 'org.springframework.scheduling.annotation.ScheduledAnnotationReactiveSupportTests$ReactiveMethods'"); + } + static class ReactiveMethods { public String oops() { @@ -137,142 +176,4 @@ public Mono monoError() { } } - - @Nested - class ReactiveTaskTests { - - private ReactiveMethods target; - - @BeforeEach - void init() { - this.target = new ReactiveMethods(); - } - - @Test - void isReactiveRejectsWithParams() { - Method m = ReflectionUtils.findMethod(ReactiveMethods.class, "monoWithParam", String.class); - - //isReactive rejects with context - assertThatIllegalArgumentException().isThrownBy(() -> isReactive(m)) - .withMessage("Reactive methods may only be annotated with @Scheduled if declared without arguments") - .withNoCause(); - - //constructor of task doesn't provide the context isReactive does - assertThatIllegalArgumentException().isThrownBy(() -> new ScheduledAnnotationReactiveSupport.ReactiveTask( - m, target, Duration.ZERO, Duration.ZERO, false)) - .withMessage("wrong number of arguments") - .withNoCause(); - } - - @Test - void rejectCantProducePublisher() { - Method m = ReflectionUtils.findMethod(ReactiveMethods.class, "monoThrows"); - - //static helper method - assertThatIllegalArgumentException().isThrownBy(() -> getPublisherFor(m, target)) - .withMessage("Cannot obtain a Publisher-convertible value from the @Scheduled reactive method") - .withCause(new IllegalStateException("expected")); - - //constructor of task - assertThatIllegalArgumentException().isThrownBy(() -> new ScheduledAnnotationReactiveSupport.ReactiveTask( - m, target, Duration.ZERO, Duration.ZERO, false)) - .withMessage("Cannot obtain a Publisher-convertible value from the @Scheduled reactive method") - .withCause(new IllegalStateException("expected")); - } - - @Test - void rejectCantAccessMethod() { - Method m = ReflectionUtils.findMethod(ReactiveMethods.class, "monoThrowsIllegalAccess"); - - //static helper method - assertThatIllegalArgumentException().isThrownBy(() -> getPublisherFor(m, target)) - .withMessage("Cannot obtain a Publisher-convertible value from the @Scheduled reactive method") - .withCause(new IllegalAccessException("expected")); - - //constructor of task - assertThatIllegalArgumentException().isThrownBy(() -> new ScheduledAnnotationReactiveSupport.ReactiveTask( - m, target, Duration.ZERO, Duration.ZERO, false)) - .withMessage("Cannot obtain a Publisher-convertible value from the @Scheduled reactive method") - .withCause(new IllegalAccessException("expected")); - } - - @Test - void hasCheckpointToString() { - Method m = ReflectionUtils.findMethod(ReactiveMethods.class, "mono"); - final ScheduledAnnotationReactiveSupport.ReactiveTask reactiveTask = new ScheduledAnnotationReactiveSupport.ReactiveTask( - m, target, Duration.ZERO, Duration.ZERO, false); - - assertThat(reactiveTask).hasToString("@Scheduled 'mono()' in bean 'org.springframework.scheduling.annotation.ScheduledAnnotationReactiveSupportTests$ReactiveMethods'"); - } - - @Test - void cancelledEarlyPreventsSubscription() { - Method m = ReflectionUtils.findMethod(ReactiveMethods.class, "trackingMono"); - final ScheduledAnnotationReactiveSupport.ReactiveTask reactiveTask = new ScheduledAnnotationReactiveSupport.ReactiveTask( - m, target, Duration.ZERO, Duration.ofSeconds(10), false); - reactiveTask.cancel(); - reactiveTask.subscribe(); - - assertThat(target.subscription).hasValue(0); - } - - @Test - void noInitialDelayFixedDelay() throws InterruptedException { - Method m = ReflectionUtils.findMethod(target.getClass(), "trackingMono"); - final ScheduledAnnotationReactiveSupport.ReactiveTask reactiveTask = new ScheduledAnnotationReactiveSupport.ReactiveTask( - m, target, Duration.ZERO, Duration.ofSeconds(10), false); - reactiveTask.subscribe(); - Thread.sleep(500); - reactiveTask.cancel(); - - assertThat(target.subscription).hasValue(1); - } - - @Test - void noInitialDelayFixedRate() throws InterruptedException { - Method m = ReflectionUtils.findMethod(target.getClass(), "trackingMono"); - final ScheduledAnnotationReactiveSupport.ReactiveTask reactiveTask = new ScheduledAnnotationReactiveSupport.ReactiveTask( - m, target, Duration.ZERO, Duration.ofSeconds(10), true); - reactiveTask.subscribe(); - Thread.sleep(500); - reactiveTask.cancel(); - - assertThat(target.subscription).hasValue(1); - } - - @Test - void initialDelayFixedDelay() throws InterruptedException { - Method m = ReflectionUtils.findMethod(target.getClass(), "trackingMono"); - final ScheduledAnnotationReactiveSupport.ReactiveTask reactiveTask = new ScheduledAnnotationReactiveSupport.ReactiveTask( - m, target, Duration.ofSeconds(10), Duration.ofMillis(500), false); - reactiveTask.subscribe(); - Thread.sleep(500); - reactiveTask.cancel(); - - assertThat(target.subscription).hasValue(0); - } - - @Test - void initialDelayFixedRate() throws InterruptedException { - Method m = ReflectionUtils.findMethod(target.getClass(), "trackingMono"); - final ScheduledAnnotationReactiveSupport.ReactiveTask reactiveTask = new ScheduledAnnotationReactiveSupport.ReactiveTask( - m, target, Duration.ofSeconds(10), Duration.ofMillis(500), true); - reactiveTask.subscribe(); - Thread.sleep(500); - reactiveTask.cancel(); - - assertThat(target.subscription).hasValue(0); - } - - @Test - void monoErrorHasCheckpoint() throws InterruptedException { - Method m = ReflectionUtils.findMethod(target.getClass(), "monoError"); - final ScheduledAnnotationReactiveSupport.ReactiveTask reactiveTask = new ScheduledAnnotationReactiveSupport.ReactiveTask( - m, target, Duration.ZERO, Duration.ofSeconds(10), true); - - assertThat(reactiveTask.checkpoint).isEqualTo("@Scheduled 'monoError()' in bean " - + "'org.springframework.scheduling.annotation.ScheduledAnnotationReactiveSupportTests$ReactiveMethods'"); - } - - } } From 78a47c9cb498bbfce34a8ee7fd5bfef1fa7a8c3f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Basl=C3=A9?= Date: Tue, 25 Apr 2023 15:55:49 +0200 Subject: [PATCH 16/19] polish and fix tests --- .../ScheduledAnnotationReactiveSupport.java | 14 +-- ...heduledAnnotationReactiveSupportTests.java | 11 ++- ...ScheduledAnnotationReactiveSupportTests.kt | 89 ------------------- 3 files changed, 16 insertions(+), 98 deletions(-) diff --git a/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupport.java b/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupport.java index 47fb6a0278c7..371fbc674e43 100644 --- a/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupport.java +++ b/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupport.java @@ -71,7 +71,7 @@ static boolean isReactive(Method method) { Assert.isTrue(method.getParameterCount() == 1,"Kotlin suspending functions may only be" + " annotated with @Scheduled if declared without arguments"); Assert.isTrue(coroutinesReactorPresent, "Kotlin suspending functions may only be annotated with" - + " @Scheduled if the Coroutine-Reactor bridge (kotlinx.coroutines.reactive) is present at runtime"); + + " @Scheduled if the Coroutine-Reactor bridge (kotlinx.coroutines.reactor) is present at runtime"); return true; } ReactiveAdapterRegistry registry = ReactiveAdapterRegistry.getSharedInstance(); @@ -165,8 +165,8 @@ public void onNext(Object o) { } @Override - public void onError(Throwable e) { - LOGGER.warn("Unexpected error occurred in scheduled reactive task", e); + public void onError(Throwable ex) { + LOGGER.warn("Unexpected error occurred in scheduled reactive task", ex); latch.countDown(); } @@ -178,8 +178,8 @@ public void onComplete() { try { latch.await(); } - catch (InterruptedException e) { - throw new RuntimeException(e); + catch (InterruptedException ex) { + throw new RuntimeException(ex); } }; } @@ -195,8 +195,8 @@ public void onNext(Object o) { } @Override - public void onError(Throwable e) { - LOGGER.warn("Unexpected error occurred in scheduled reactive task", e); + public void onError(Throwable ex) { + LOGGER.warn("Unexpected error occurred in scheduled reactive task", ex); } @Override diff --git a/spring-context/src/test/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupportTests.java b/spring-context/src/test/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupportTests.java index af3f6ee83eb2..639672f4d9f1 100644 --- a/spring-context/src/test/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupportTests.java +++ b/spring-context/src/test/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupportTests.java @@ -105,8 +105,15 @@ void rejectCantAccessMethod() { @Test void hasCheckpointToString() { - //FIXME test checkpointing - assertThat("FIXME").isEqualTo("@Scheduled 'mono()' in bean 'org.springframework.scheduling.annotation.ScheduledAnnotationReactiveSupportTests$ReactiveMethods'"); + ReactiveMethods target = new ReactiveMethods(); + Method m = ReflectionUtils.findMethod(ReactiveMethods.class, "mono"); + Publisher p = getPublisherFor(m, target); + + assertThat(p.getClass().getName()) + .as("checkpoint class") + .isEqualTo("reactor.core.publisher.FluxOnAssembly"); + + assertThat(p).hasToString("checkpoint(\"@Scheduled 'mono()' in bean 'org.springframework.scheduling.annotation.ScheduledAnnotationReactiveSupportTests$ReactiveMethods'\")"); } static class ReactiveMethods { diff --git a/spring-context/src/test/kotlin/org/springframework/scheduling/annotation/KotlinScheduledAnnotationReactiveSupportTests.kt b/spring-context/src/test/kotlin/org/springframework/scheduling/annotation/KotlinScheduledAnnotationReactiveSupportTests.kt index 7989386494be..51cf0e5481a8 100644 --- a/spring-context/src/test/kotlin/org/springframework/scheduling/annotation/KotlinScheduledAnnotationReactiveSupportTests.kt +++ b/spring-context/src/test/kotlin/org/springframework/scheduling/annotation/KotlinScheduledAnnotationReactiveSupportTests.kt @@ -26,12 +26,10 @@ import org.junit.jupiter.api.BeforeEach import org.junit.jupiter.api.Test import org.junit.jupiter.params.ParameterizedTest import org.junit.jupiter.params.provider.ValueSource -import org.springframework.scheduling.annotation.ScheduledAnnotationReactiveSupport.ReactiveTask import org.springframework.scheduling.annotation.ScheduledAnnotationReactiveSupport.getPublisherFor import org.springframework.scheduling.annotation.ScheduledAnnotationReactiveSupport.isReactive import org.springframework.util.ReflectionUtils import reactor.core.publisher.Mono -import java.time.Duration import java.util.concurrent.atomic.AtomicInteger import kotlin.coroutines.Continuation @@ -122,11 +120,6 @@ class KotlinScheduledAnnotationReactiveSupportTests { Assertions.assertThatIllegalArgumentException().isThrownBy { isReactive(m) } .withMessage("Kotlin suspending functions may only be annotated with @Scheduled if declared without arguments") .withNoCause() - - //constructor of task doesn't reject - Assertions.assertThatNoException().isThrownBy { - ReactiveTask(m, target!!, Duration.ZERO, Duration.ZERO, false) - } } @Test @@ -137,13 +130,6 @@ class KotlinScheduledAnnotationReactiveSupportTests { Assertions.assertThatIllegalArgumentException().isThrownBy { getPublisherFor(m!!, target!!) } .withMessage("Cannot convert the @Scheduled reactive method return type to Publisher") .withNoCause() - - //constructor of task - Assertions.assertThatIllegalArgumentException().isThrownBy { - ReactiveTask(m!!, target!!, Duration.ZERO, Duration.ZERO, false) - } - .withMessage("Cannot convert the @Scheduled reactive method return type to Publisher") - .withNoCause() } @Test @@ -155,11 +141,6 @@ class KotlinScheduledAnnotationReactiveSupportTests { Assertions.assertThatIllegalStateException().isThrownBy { mono.block() } .withMessage("expected") .withNoCause() - - //constructor of task doesn't throw - Assertions.assertThatNoException().isThrownBy { - ReactiveTask(m, target!!, Duration.ZERO, Duration.ZERO, false) - } } @Test @@ -171,74 +152,4 @@ class KotlinScheduledAnnotationReactiveSupportTests { mono.block() assertThat(target!!.subscription).describedAs("after subscription").hasValue(1) } - - @Test - fun hasCheckpointToString() { - val m = ReflectionUtils.findMethod(SuspendingFunctions::class.java, "suspending", Continuation::class.java) - val reactiveTask = ReactiveTask(m!!, target!!, Duration.ZERO, Duration.ZERO, false) - assertThat(reactiveTask).hasToString("@Scheduled 'suspending()' in bean 'org.springframework.scheduling.annotation.KotlinScheduledAnnotationReactiveSupportTests\$SuspendingFunctions'") - } - - @Test - fun cancelledEarlyPreventsSubscription() { - val m = ReflectionUtils.findMethod(SuspendingFunctions::class.java, "suspendingTracking", Continuation::class.java) - val reactiveTask = ReactiveTask(m!!, target!!, Duration.ZERO, Duration.ofSeconds(10), false) - reactiveTask.cancel() - reactiveTask.subscribe() - assertThat(target!!.subscription).hasValue(0) - } - - @Test - fun multipleSubscriptionsTracked() { - val m = ReflectionUtils.findMethod(SuspendingFunctions::class.java, "suspendingTracking", Continuation::class.java) - val reactiveTask = ReactiveTask(m!!, target!!, Duration.ZERO, Duration.ofMillis(500), false) - reactiveTask.subscribe() - Thread.sleep(1500) - reactiveTask.cancel() - assertThat(target!!.subscription).hasValueGreaterThanOrEqualTo(3) - } - - @Test - @Throws(InterruptedException::class) - fun noInitialDelayFixedDelay() { - val m = ReflectionUtils.findMethod(SuspendingFunctions::class.java, "suspendingTracking", Continuation::class.java) - val reactiveTask = ReactiveTask(m!!, target!!, Duration.ZERO, Duration.ofSeconds(10), false) - reactiveTask.subscribe() - Thread.sleep(500) - reactiveTask.cancel() - assertThat(target!!.subscription).hasValue(1) - } - - @Test - @Throws(InterruptedException::class) - fun noInitialDelayFixedRate() { - val m = ReflectionUtils.findMethod(SuspendingFunctions::class.java, "suspendingTracking", Continuation::class.java) - val reactiveTask = ReactiveTask(m!!, target!!, Duration.ZERO, Duration.ofSeconds(10), true) - reactiveTask.subscribe() - Thread.sleep(500) - reactiveTask.cancel() - assertThat(target!!.subscription).hasValue(1) - } - - @Test - @Throws(InterruptedException::class) - fun initialDelayFixedDelay() { - val m = ReflectionUtils.findMethod(SuspendingFunctions::class.java, "suspendingTracking", Continuation::class.java) - val reactiveTask = ReactiveTask(m!!, target!!, Duration.ofSeconds(10), Duration.ofMillis(500), false) - reactiveTask.subscribe() - Thread.sleep(500) - reactiveTask.cancel() - assertThat(target!!.subscription).hasValue(0) - } - - @Test - @Throws(InterruptedException::class) - fun initialDelayFixedRate() { - val m = ReflectionUtils.findMethod(SuspendingFunctions::class.java, "suspendingTracking", Continuation::class.java) - val reactiveTask = ReactiveTask(m!!, target!!, Duration.ofSeconds(10), Duration.ofMillis(500), true) - reactiveTask.subscribe() - Thread.sleep(500) - reactiveTask.cancel() - assertThat(target!!.subscription).hasValue(0) - } } \ No newline at end of file From 1f871110109d37c398f701891b0a124b344b00e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Basl=C3=A9?= Date: Tue, 25 Apr 2023 18:49:26 +0200 Subject: [PATCH 17/19] Track active subscriptions as Runnable in the processor --- .../ScheduledAnnotationBeanPostProcessor.java | 25 +++- .../ScheduledAnnotationReactiveSupport.java | 117 ++++++++++++------ 2 files changed, 99 insertions(+), 43 deletions(-) diff --git a/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationBeanPostProcessor.java b/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationBeanPostProcessor.java index 4a894bcce02c..01a5244d8459 100644 --- a/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationBeanPostProcessor.java +++ b/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationBeanPostProcessor.java @@ -28,6 +28,7 @@ import java.util.Set; import java.util.TimeZone; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.TimeUnit; @@ -144,6 +145,8 @@ public class ScheduledAnnotationBeanPostProcessor private final Map> scheduledTasks = new IdentityHashMap<>(16); + private final Map> reactiveSubscriptions = new IdentityHashMap<>(16); + /** * Create a default {@code ScheduledAnnotationBeanPostProcessor}. @@ -573,8 +576,8 @@ protected void processScheduledSync(Scheduled scheduled, Method method, Object b protected void processScheduledAsync(Scheduled scheduled, Method method, Object bean) { Runnable task; try { - boolean isFixedDelaySpecialCase = scheduled.fixedDelay() > 0 || StringUtils.hasText(scheduled.fixedDelayString()); - task = ScheduledAnnotationReactiveSupport.createSubscriptionRunnable(method, bean, isFixedDelaySpecialCase); + task = ScheduledAnnotationReactiveSupport.createSubscriptionRunnable(method, bean, scheduled, + this.reactiveSubscriptions.computeIfAbsent(bean, k -> new CopyOnWriteArrayList<>())); } catch (IllegalArgumentException ex) { throw new IllegalStateException("Could not create recurring task for @Scheduled method '" + method.getName() + "': " + ex.getMessage()); @@ -620,7 +623,8 @@ private static boolean isP(char ch) { /** * Return all currently scheduled tasks, from {@link Scheduled} methods * as well as from programmatic {@link SchedulingConfigurer} interaction. - *

Note this doesn't include scheduled reactive methods. + *

Note this includes upcoming scheduled subscriptions for reactive methods, + * but doesn't cover any currently active subscription for such methods. * @since 5.0.2 */ @Override @@ -639,20 +643,27 @@ public Set getScheduledTasks() { @Override public void postProcessBeforeDestruction(Object bean, String beanName) { Set tasks; + List liveSubscriptions; synchronized (this.scheduledTasks) { tasks = this.scheduledTasks.remove(bean); + liveSubscriptions = this.reactiveSubscriptions.remove(bean); } if (tasks != null) { for (ScheduledTask task : tasks) { task.cancel(); } } + if (liveSubscriptions != null) { + for (Runnable subscription : liveSubscriptions) { + subscription.run(); // equivalent to cancelling the subscription + } + } } @Override public boolean requiresDestruction(Object bean) { synchronized (this.scheduledTasks) { - return this.scheduledTasks.containsKey(bean); + return this.scheduledTasks.containsKey(bean) || this.reactiveSubscriptions.containsKey(bean); } } @@ -666,6 +677,12 @@ public void destroy() { } } this.scheduledTasks.clear(); + Collection> allLiveSubscriptions = this.reactiveSubscriptions.values(); + for (List liveSubscriptions : allLiveSubscriptions) { + for (Runnable liveSubscription : liveSubscriptions) { + liveSubscription.run(); //equivalent to cancelling the subscription + } + } } this.registrar.destroy(); } diff --git a/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupport.java b/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupport.java index 371fbc674e43..410cf50e3af2 100644 --- a/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupport.java +++ b/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupport.java @@ -18,6 +18,7 @@ import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; +import java.util.List; import java.util.concurrent.CountDownLatch; import org.apache.commons.logging.Log; @@ -32,9 +33,11 @@ import org.springframework.core.KotlinDetector; import org.springframework.core.ReactiveAdapter; import org.springframework.core.ReactiveAdapterRegistry; +import org.springframework.lang.Nullable; import org.springframework.util.Assert; import org.springframework.util.ClassUtils; import org.springframework.util.ReflectionUtils; +import org.springframework.util.StringUtils; /** * Helper class for @{@link ScheduledAnnotationBeanPostProcessor} to support reactive cases @@ -148,33 +151,16 @@ static Publisher getPublisherFor(Method method, Object bean) { * (i.e. the task blocks until completion of the Publisher, then the delay is applied * until next iteration). */ - static Runnable createSubscriptionRunnable(Method method, Object targetBean, boolean isFixedDelaySpecialCase) { + static Runnable createSubscriptionRunnable(Method method, Object targetBean, Scheduled scheduled, + List subscriptionTrackerRegistry) { + boolean shouldBlock = scheduled.fixedDelay() > 0 || StringUtils.hasText(scheduled.fixedDelayString()); final Publisher publisher = getPublisherFor(method, targetBean); - if (isFixedDelaySpecialCase) { + if (shouldBlock) { return () -> { final CountDownLatch latch = new CountDownLatch(1); - publisher.subscribe(new Subscriber() { - @Override - public void onSubscribe(Subscription s) { - s.request(Integer.MAX_VALUE); - } - - @Override - public void onNext(Object o) { - // NO-OP - } - - @Override - public void onError(Throwable ex) { - LOGGER.warn("Unexpected error occurred in scheduled reactive task", ex); - latch.countDown(); - } - - @Override - public void onComplete() { - latch.countDown(); - } - }); + TrackingSubscriber subscriber = new TrackingSubscriber(subscriptionTrackerRegistry, latch); + subscriptionTrackerRegistry.add(subscriber); + publisher.subscribe(subscriber); try { latch.await(); } @@ -183,27 +169,80 @@ public void onComplete() { } }; } - return () -> publisher.subscribe(new Subscriber() { - @Override - public void onSubscribe(Subscription s) { - s.request(Integer.MAX_VALUE); - } + return () -> { + final TrackingSubscriber subscriber = new TrackingSubscriber(subscriptionTrackerRegistry); + subscriptionTrackerRegistry.add(subscriber); + publisher.subscribe(subscriber); + }; + } + + /** + * A {@code Subscriber} which keeps track of its {@code Subscription} and exposes the + * capacity to cancel the subscription as a {@code Runnable}. Can optionally support + * blocking if a {@code CountDownLatch} is passed at construction. + */ + private static final class TrackingSubscriber implements Subscriber, Runnable { + + private final List subscriptionTrackerRegistry; + + @Nullable + private final CountDownLatch blockingLatch; + + /* + Implementation note: since this is created last minute when subscribing, + there shouldn't be a way to cancel the tracker externally from the + ScheduledAnnotationBeanProcessor before the #setSubscription(Subscription) + method is called. + */ + @Nullable + private Subscription s; + + TrackingSubscriber(List subscriptionTrackerRegistry) { + this(subscriptionTrackerRegistry, null); + } - @Override - public void onNext(Object o) { - // NO-OP + TrackingSubscriber(List subscriptionTrackerRegistry, @Nullable CountDownLatch latch) { + this.subscriptionTrackerRegistry = subscriptionTrackerRegistry; + this.blockingLatch = latch; + } + + @Override + public void run() { + if (this.s != null) { + this.s.cancel(); + } + if (this.blockingLatch != null) { + this.blockingLatch.countDown(); } + } + + @Override + public void onSubscribe(Subscription s) { + this.s = s; + s.request(Integer.MAX_VALUE); + } + + @Override + public void onNext(Object o) { + // NO-OP + } - @Override - public void onError(Throwable ex) { - LOGGER.warn("Unexpected error occurred in scheduled reactive task", ex); + @Override + public void onError(Throwable ex) { + this.subscriptionTrackerRegistry.remove(this); + LOGGER.warn("Unexpected error occurred in scheduled reactive task", ex); + if (this.blockingLatch != null) { + this.blockingLatch.countDown(); } + } - @Override - public void onComplete() { - // NO-OP + @Override + public void onComplete() { + this.subscriptionTrackerRegistry.remove(this); + if (this.blockingLatch != null) { + this.blockingLatch.countDown(); } - }); + } } } From 5b3ae76288ffb2659ece3919f30b064b29bc8c87 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Basl=C3=A9?= Date: Tue, 25 Apr 2023 19:00:48 +0200 Subject: [PATCH 18/19] polish javadoc and fixmes --- .../modules/ROOT/pages/integration/scheduling.adoc | 7 +++---- .../springframework/scheduling/annotation/Scheduled.java | 3 +-- .../ScheduledAnnotationReactiveSupportTests.java | 4 +--- 3 files changed, 5 insertions(+), 9 deletions(-) diff --git a/framework-docs/modules/ROOT/pages/integration/scheduling.adoc b/framework-docs/modules/ROOT/pages/integration/scheduling.adoc index a30aa5da4601..e8f723af0c7b 100644 --- a/framework-docs/modules/ROOT/pages/integration/scheduling.adoc +++ b/framework-docs/modules/ROOT/pages/integration/scheduling.adoc @@ -499,10 +499,9 @@ seconds: [NOTE] ==== When destroying the annotated bean or closing the application context Spring Framework cancels -scheduled tasks, which includes the next scheduled subscription to the `Publisher`. -However, there is no tracking of individual subscriptions: once a subscription has started -it can't be cancelled. For that reason, it is a pre-requisite that the `Publisher` is finite -in addition to supporting multiple subsequent subscriptions. +scheduled tasks, which includes the next scheduled subscription to the `Publisher` as well +as any past subscription that is still currently active (e.g. for long-running publishers, +or even infinite publishers). ==== diff --git a/spring-context/src/main/java/org/springframework/scheduling/annotation/Scheduled.java b/spring-context/src/main/java/org/springframework/scheduling/annotation/Scheduled.java index 6e9ddc579b2f..5f2fe23b0a3a 100644 --- a/spring-context/src/main/java/org/springframework/scheduling/annotation/Scheduled.java +++ b/spring-context/src/main/java/org/springframework/scheduling/annotation/Scheduled.java @@ -38,8 +38,7 @@ * *

Methods that return a reactive {@code Publisher} or a type which can be adapted * to {@code Publisher} by the default {@code ReactiveAdapterRegistry} are supported. - * The {@code Publisher} MUST be finite and MUST support multiple subsequent subscriptions - * (i.e. be cold). + * The {@code Publisher} MUST support multiple subsequent subscriptions (i.e. be cold). * The returned Publisher is only produced once, and the scheduling infrastructure * then periodically {@code subscribe()} to it according to configuration. * Values emitted by the publisher are ignored. Errors are logged at WARN level, which diff --git a/spring-context/src/test/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupportTests.java b/spring-context/src/test/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupportTests.java index 639672f4d9f1..67a80aace1fa 100644 --- a/spring-context/src/test/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupportTests.java +++ b/spring-context/src/test/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupportTests.java @@ -99,9 +99,7 @@ void rejectCantAccessMethod() { .withCause(new IllegalAccessException("expected")); } - //FIXME find a way to test the case with fixedDelay effectively turning into a fixedRate ? - - //FIXME test createCheckpointPublisherFor uses Reactor and checkpoint operator + //TODO find a way to test the case with fixedDelay effectively turning into a fixedRate ? @Test void hasCheckpointToString() { From ae3f20843cd8d67073844b26676e1e1dba69d9e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Basl=C3=A9?= Date: Mon, 15 May 2023 17:28:41 +0200 Subject: [PATCH 19/19] improve testability of the subscribing runnables --- .../ScheduledAnnotationReactiveSupport.java | 42 ++++++++++---- ...heduledAnnotationReactiveSupportTests.java | 56 ++++++++++++++++++- 2 files changed, 86 insertions(+), 12 deletions(-) diff --git a/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupport.java b/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupport.java index 410cf50e3af2..f5998132d319 100644 --- a/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupport.java +++ b/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupport.java @@ -155,25 +155,45 @@ static Runnable createSubscriptionRunnable(Method method, Object targetBean, Sch List subscriptionTrackerRegistry) { boolean shouldBlock = scheduled.fixedDelay() > 0 || StringUtils.hasText(scheduled.fixedDelayString()); final Publisher publisher = getPublisherFor(method, targetBean); - if (shouldBlock) { - return () -> { + return new SubscribingRunnable(publisher, shouldBlock, subscriptionTrackerRegistry); + } + + /** + * Utility implementation of {@code Runnable} that subscribes to a {@code Publisher} + * or subscribes-then-blocks if {@code shouldBlock} is set to {@code true}. + */ + static final class SubscribingRunnable implements Runnable { + + final Publisher publisher; + final boolean shouldBlock; + final List subscriptionTrackerRegistry; + + SubscribingRunnable(Publisher publisher, boolean shouldBlock, List subscriptionTrackerRegistry) { + this.publisher = publisher; + this.shouldBlock = shouldBlock; + this.subscriptionTrackerRegistry = subscriptionTrackerRegistry; + } + + @Override + public void run() { + if (this.shouldBlock) { final CountDownLatch latch = new CountDownLatch(1); - TrackingSubscriber subscriber = new TrackingSubscriber(subscriptionTrackerRegistry, latch); - subscriptionTrackerRegistry.add(subscriber); - publisher.subscribe(subscriber); + TrackingSubscriber subscriber = new TrackingSubscriber(this.subscriptionTrackerRegistry, latch); + this.subscriptionTrackerRegistry.add(subscriber); + this.publisher.subscribe(subscriber); try { latch.await(); } catch (InterruptedException ex) { throw new RuntimeException(ex); } - }; + } + else { + final TrackingSubscriber subscriber = new TrackingSubscriber(this.subscriptionTrackerRegistry); + this.subscriptionTrackerRegistry.add(subscriber); + this.publisher.subscribe(subscriber); + } } - return () -> { - final TrackingSubscriber subscriber = new TrackingSubscriber(subscriptionTrackerRegistry); - subscriptionTrackerRegistry.add(subscriber); - publisher.subscribe(subscriber); - }; } /** diff --git a/spring-context/src/test/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupportTests.java b/spring-context/src/test/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupportTests.java index 67a80aace1fa..d5357c35174f 100644 --- a/spring-context/src/test/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupportTests.java +++ b/spring-context/src/test/java/org/springframework/scheduling/annotation/ScheduledAnnotationReactiveSupportTests.java @@ -17,6 +17,9 @@ package org.springframework.scheduling.annotation; import java.lang.reflect.Method; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; import java.util.concurrent.CompletableFuture; import java.util.concurrent.atomic.AtomicInteger; @@ -29,10 +32,12 @@ import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; +import org.springframework.core.annotation.AnnotationUtils; import org.springframework.util.ReflectionUtils; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; +import static org.springframework.scheduling.annotation.ScheduledAnnotationReactiveSupport.createSubscriptionRunnable; import static org.springframework.scheduling.annotation.ScheduledAnnotationReactiveSupport.getPublisherFor; import static org.springframework.scheduling.annotation.ScheduledAnnotationReactiveSupport.isReactive; @@ -99,7 +104,56 @@ void rejectCantAccessMethod() { .withCause(new IllegalAccessException("expected")); } - //TODO find a way to test the case with fixedDelay effectively turning into a fixedRate ? + @Test + void fixedDelayIsBlocking() { + ReactiveMethods target = new ReactiveMethods(); + Method m = ReflectionUtils.findMethod(ReactiveMethods.class, "mono"); + Scheduled fixedDelayString = AnnotationUtils.synthesizeAnnotation(Map.of("fixedDelayString", "123"), Scheduled.class, null); + Scheduled fixedDelayLong = AnnotationUtils.synthesizeAnnotation(Map.of("fixedDelay", 123L), Scheduled.class, null); + List tracker = new ArrayList<>(); + + assertThat(createSubscriptionRunnable(m, target, fixedDelayString, tracker)) + .isInstanceOfSatisfying(ScheduledAnnotationReactiveSupport.SubscribingRunnable.class, sr -> + assertThat(sr.shouldBlock).as("fixedDelayString.shouldBlock").isTrue() + ); + + assertThat(createSubscriptionRunnable(m, target, fixedDelayLong, tracker)) + .isInstanceOfSatisfying(ScheduledAnnotationReactiveSupport.SubscribingRunnable.class, sr -> + assertThat(sr.shouldBlock).as("fixedDelayLong.shouldBlock").isTrue() + ); + } + + @Test + void fixedRateIsNotBlocking() { + ReactiveMethods target = new ReactiveMethods(); + Method m = ReflectionUtils.findMethod(ReactiveMethods.class, "mono"); + Scheduled fixedRateString = AnnotationUtils.synthesizeAnnotation(Map.of("fixedRateString", "123"), Scheduled.class, null); + Scheduled fixedRateLong = AnnotationUtils.synthesizeAnnotation(Map.of("fixedRate", 123L), Scheduled.class, null); + List tracker = new ArrayList<>(); + + assertThat(createSubscriptionRunnable(m, target, fixedRateString, tracker)) + .isInstanceOfSatisfying(ScheduledAnnotationReactiveSupport.SubscribingRunnable.class, sr -> + assertThat(sr.shouldBlock).as("fixedRateString.shouldBlock").isFalse() + ); + + assertThat(createSubscriptionRunnable(m, target, fixedRateLong, tracker)) + .isInstanceOfSatisfying(ScheduledAnnotationReactiveSupport.SubscribingRunnable.class, sr -> + assertThat(sr.shouldBlock).as("fixedRateLong.shouldBlock").isFalse() + ); + } + + @Test + void cronIsNotBlocking() { + ReactiveMethods target = new ReactiveMethods(); + Method m = ReflectionUtils.findMethod(ReactiveMethods.class, "mono"); + Scheduled cron = AnnotationUtils.synthesizeAnnotation(Map.of("cron", "-"), Scheduled.class, null); + List tracker = new ArrayList<>(); + + assertThat(createSubscriptionRunnable(m, target, cron, tracker)) + .isInstanceOfSatisfying(ScheduledAnnotationReactiveSupport.SubscribingRunnable.class, sr -> + assertThat(sr.shouldBlock).as("cron.shouldBlock").isFalse() + ); + } @Test void hasCheckpointToString() {