From b78aed99ea7ebb195d5439d569b2ba797e57a2cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Nicoll?= Date: Fri, 8 Dec 2023 15:06:55 +0100 Subject: [PATCH] Provide a dedicated exception if ScheduledExecutor is not configured While it is not easily possible to create a ConcurrentTaskScheduler with no scheduled executor, DefaultManagedTaskScheduler has a default constructor that does that as the JNDI lookup happens in the regular afterPropertiesSet callback. If such an instance is created manually, it can throw an unhelpful NullPointerException if one attempts to schedule a task. This commit updates ConcurrentTaskScheduler to flag that the scheduled executor could be `null` and check if that's the case before using it. This now throws a dedicated exception that should hopefully provide better context for those upgrading. See gh-27914 Closes gh-31751 --- .../concurrent/ConcurrentTaskScheduler.java | 40 ++++++--- .../DefaultManagedTaskScheduler.java | 2 + .../DefaultManagedTaskSchedulerTests.java | 86 +++++++++++++++++++ 3 files changed, 115 insertions(+), 13 deletions(-) create mode 100644 spring-context/src/test/java/org/springframework/scheduling/concurrent/DefaultManagedTaskSchedulerTests.java diff --git a/spring-context/src/main/java/org/springframework/scheduling/concurrent/ConcurrentTaskScheduler.java b/spring-context/src/main/java/org/springframework/scheduling/concurrent/ConcurrentTaskScheduler.java index 88f36184efd9..01770c2c1975 100644 --- a/spring-context/src/main/java/org/springframework/scheduling/concurrent/ConcurrentTaskScheduler.java +++ b/spring-context/src/main/java/org/springframework/scheduling/concurrent/ConcurrentTaskScheduler.java @@ -89,6 +89,7 @@ public class ConcurrentTaskScheduler extends ConcurrentTaskExecutor implements T } + @Nullable private ScheduledExecutorService scheduledExecutor; private boolean enterpriseConcurrentScheduler = false; @@ -168,6 +169,13 @@ public void setScheduledExecutor(ScheduledExecutorService scheduledExecutor) { initScheduledExecutor(scheduledExecutor); } + private ScheduledExecutorService getScheduledExecutor() { + if (this.scheduledExecutor == null) { + throw new IllegalStateException("No ScheduledExecutor is configured"); + } + return this.scheduledExecutor; + } + /** * Provide an {@link ErrorHandler} strategy. */ @@ -195,6 +203,7 @@ public Clock getClock() { @Override @Nullable public ScheduledFuture schedule(Runnable task, Trigger trigger) { + ScheduledExecutorService scheduleExecutorToUse = getScheduledExecutor(); try { if (this.enterpriseConcurrentScheduler) { return new EnterpriseConcurrentTriggerScheduler().schedule(decorateTask(task, true), trigger); @@ -202,68 +211,73 @@ public ScheduledFuture schedule(Runnable task, Trigger trigger) { else { ErrorHandler errorHandler = (this.errorHandler != null ? this.errorHandler : TaskUtils.getDefaultErrorHandler(true)); - return new ReschedulingRunnable(task, trigger, this.clock, this.scheduledExecutor, errorHandler).schedule(); + return new ReschedulingRunnable(task, trigger, this.clock, scheduleExecutorToUse, errorHandler).schedule(); } } catch (RejectedExecutionException ex) { - throw new TaskRejectedException(this.scheduledExecutor, task, ex); + throw new TaskRejectedException(scheduleExecutorToUse, task, ex); } } @Override public ScheduledFuture schedule(Runnable task, Instant startTime) { + ScheduledExecutorService scheduleExecutorToUse = getScheduledExecutor(); Duration delay = Duration.between(this.clock.instant(), startTime); try { - return this.scheduledExecutor.schedule(decorateTask(task, false), NANO.convert(delay), NANO); + return scheduleExecutorToUse.schedule(decorateTask(task, false), NANO.convert(delay), NANO); } catch (RejectedExecutionException ex) { - throw new TaskRejectedException(this.scheduledExecutor, task, ex); + throw new TaskRejectedException(scheduleExecutorToUse, task, ex); } } @Override public ScheduledFuture scheduleAtFixedRate(Runnable task, Instant startTime, Duration period) { + ScheduledExecutorService scheduleExecutorToUse = getScheduledExecutor(); Duration initialDelay = Duration.between(this.clock.instant(), startTime); try { - return this.scheduledExecutor.scheduleAtFixedRate(decorateTask(task, true), + return scheduleExecutorToUse.scheduleAtFixedRate(decorateTask(task, true), NANO.convert(initialDelay), NANO.convert(period), NANO); } catch (RejectedExecutionException ex) { - throw new TaskRejectedException(this.scheduledExecutor, task, ex); + throw new TaskRejectedException(scheduleExecutorToUse, task, ex); } } @Override public ScheduledFuture scheduleAtFixedRate(Runnable task, Duration period) { + ScheduledExecutorService scheduleExecutorToUse = getScheduledExecutor(); try { - return this.scheduledExecutor.scheduleAtFixedRate(decorateTask(task, true), + return scheduleExecutorToUse.scheduleAtFixedRate(decorateTask(task, true), 0, NANO.convert(period), NANO); } catch (RejectedExecutionException ex) { - throw new TaskRejectedException(this.scheduledExecutor, task, ex); + throw new TaskRejectedException(scheduleExecutorToUse, task, ex); } } @Override public ScheduledFuture scheduleWithFixedDelay(Runnable task, Instant startTime, Duration delay) { + ScheduledExecutorService scheduleExecutorToUse = getScheduledExecutor(); Duration initialDelay = Duration.between(this.clock.instant(), startTime); try { - return this.scheduledExecutor.scheduleWithFixedDelay(decorateTask(task, true), + return scheduleExecutorToUse.scheduleWithFixedDelay(decorateTask(task, true), NANO.convert(initialDelay), NANO.convert(delay), NANO); } catch (RejectedExecutionException ex) { - throw new TaskRejectedException(this.scheduledExecutor, task, ex); + throw new TaskRejectedException(scheduleExecutorToUse, task, ex); } } @Override public ScheduledFuture scheduleWithFixedDelay(Runnable task, Duration delay) { + ScheduledExecutorService scheduleExecutorToUse = getScheduledExecutor(); try { - return this.scheduledExecutor.scheduleWithFixedDelay(decorateTask(task, true), + return scheduleExecutorToUse.scheduleWithFixedDelay(decorateTask(task, true), 0, NANO.convert(delay), NANO); } catch (RejectedExecutionException ex) { - throw new TaskRejectedException(this.scheduledExecutor, task, ex); + throw new TaskRejectedException(scheduleExecutorToUse, task, ex); } } @@ -283,7 +297,7 @@ private Runnable decorateTask(Runnable task, boolean isRepeatingTask) { private class EnterpriseConcurrentTriggerScheduler { public ScheduledFuture schedule(Runnable task, Trigger trigger) { - ManagedScheduledExecutorService executor = (ManagedScheduledExecutorService) scheduledExecutor; + ManagedScheduledExecutorService executor = (ManagedScheduledExecutorService) getScheduledExecutor(); return executor.schedule(task, new TriggerAdapter(trigger)); } diff --git a/spring-context/src/main/java/org/springframework/scheduling/concurrent/DefaultManagedTaskScheduler.java b/spring-context/src/main/java/org/springframework/scheduling/concurrent/DefaultManagedTaskScheduler.java index 5ac8ead64e51..b41315f86d55 100644 --- a/spring-context/src/main/java/org/springframework/scheduling/concurrent/DefaultManagedTaskScheduler.java +++ b/spring-context/src/main/java/org/springframework/scheduling/concurrent/DefaultManagedTaskScheduler.java @@ -28,6 +28,8 @@ /** * JNDI-based variant of {@link ConcurrentTaskScheduler}, performing a default lookup for * JSR-236's "java:comp/DefaultManagedScheduledExecutorService" in a Jakarta EE environment. + * Expected to be exposed as a bean, in particular as the default lookup happens in the + * standard {@link InitializingBean#afterPropertiesSet()} callback. * *

Note: This class is not strictly JSR-236 based; it can work with any regular * {@link java.util.concurrent.ScheduledExecutorService} that can be found in JNDI. diff --git a/spring-context/src/test/java/org/springframework/scheduling/concurrent/DefaultManagedTaskSchedulerTests.java b/spring-context/src/test/java/org/springframework/scheduling/concurrent/DefaultManagedTaskSchedulerTests.java new file mode 100644 index 000000000000..9f161418874a --- /dev/null +++ b/spring-context/src/test/java/org/springframework/scheduling/concurrent/DefaultManagedTaskSchedulerTests.java @@ -0,0 +1,86 @@ +/* + * 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.concurrent; + +import java.time.Duration; +import java.time.Instant; +import java.time.temporal.ChronoUnit; + +import org.assertj.core.api.ThrowableAssert.ThrowingCallable; +import org.junit.jupiter.api.Test; + +import org.springframework.scheduling.Trigger; + +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.mockito.Mockito.mock; + +/** + * Tests for {@link DefaultManagedTaskScheduler}. + * + * @author Stephane Nicoll + */ +class DefaultManagedTaskSchedulerTests { + + private final Runnable NO_OP = () -> {}; + + @Test + void scheduleWithTriggerAndNoScheduledExecutorProvidesDedicatedException() { + DefaultManagedTaskScheduler scheduler = new DefaultManagedTaskScheduler(); + assertNoExecutorException(() -> scheduler.schedule(NO_OP, mock(Trigger.class))); + } + + @Test + void scheduleWithInstantAndNoScheduledExecutorProvidesDedicatedException() { + DefaultManagedTaskScheduler scheduler = new DefaultManagedTaskScheduler(); + assertNoExecutorException(() -> scheduler.schedule(NO_OP, Instant.now())); + } + + @Test + void scheduleAtFixedRateWithStartTimeAndDurationAndNoScheduledExecutorProvidesDedicatedException() { + DefaultManagedTaskScheduler scheduler = new DefaultManagedTaskScheduler(); + assertNoExecutorException(() -> scheduler.scheduleAtFixedRate( + NO_OP, Instant.now(), Duration.of(1, ChronoUnit.MINUTES))); + } + + @Test + void scheduleAtFixedRateWithDurationAndNoScheduledExecutorProvidesDedicatedException() { + DefaultManagedTaskScheduler scheduler = new DefaultManagedTaskScheduler(); + assertNoExecutorException(() -> scheduler.scheduleAtFixedRate( + NO_OP, Duration.of(1, ChronoUnit.MINUTES))); + } + + @Test + void scheduleWithFixedDelayWithStartTimeAndDurationAndNoScheduledExecutorProvidesDedicatedException() { + DefaultManagedTaskScheduler scheduler = new DefaultManagedTaskScheduler(); + assertNoExecutorException(() -> scheduler.scheduleWithFixedDelay( + NO_OP, Instant.now(), Duration.of(1, ChronoUnit.MINUTES))); + } + + @Test + void scheduleWithFixedDelayWithDurationAndNoScheduledExecutorProvidesDedicatedException() { + DefaultManagedTaskScheduler scheduler = new DefaultManagedTaskScheduler(); + assertNoExecutorException(() -> scheduler.scheduleWithFixedDelay( + NO_OP, Duration.of(1, ChronoUnit.MINUTES))); + } + + private void assertNoExecutorException(ThrowingCallable callable) { + assertThatThrownBy(callable) + .isInstanceOf(IllegalStateException.class) + .hasMessage("No ScheduledExecutor is configured"); + } + +}