From 599ac58baa049d0075edf802cf056ffa7112fc87 Mon Sep 17 00:00:00 2001 From: Juergen Hoeller Date: Mon, 26 Jun 2023 19:28:08 +0200 Subject: [PATCH 1/3] Avoid arithmetic overflow for large delay/period values Closes gh-30754 --- .../concurrent/ConcurrentTaskScheduler.java | 18 +++++++---- .../concurrent/ThreadPoolTaskScheduler.java | 18 +++++++---- ...duledAnnotationBeanPostProcessorTests.java | 30 +++++++++++-------- 3 files changed, 43 insertions(+), 23 deletions(-) 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 3e580dedeb2..eb2f29a4c88 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 @@ -70,6 +70,9 @@ import org.springframework.util.ErrorHandler; */ public class ConcurrentTaskScheduler extends ConcurrentTaskExecutor implements TaskScheduler { + private static final TimeUnit NANO = TimeUnit.NANOSECONDS; + + @Nullable private static Class managedScheduledExecutorServiceClass; @@ -211,7 +214,8 @@ public class ConcurrentTaskScheduler extends ConcurrentTaskExecutor implements T public ScheduledFuture schedule(Runnable task, Instant startTime) { Duration initialDelay = Duration.between(this.clock.instant(), startTime); try { - return this.scheduledExecutor.schedule(decorateTask(task, false), initialDelay.toNanos(), TimeUnit.NANOSECONDS); + return this.scheduledExecutor.schedule(decorateTask(task, false), + NANO.convert(initialDelay), NANO); } catch (RejectedExecutionException ex) { throw new TaskRejectedException("Executor [" + this.scheduledExecutor + "] did not accept task: " + task, ex); @@ -222,7 +226,8 @@ public class ConcurrentTaskScheduler extends ConcurrentTaskExecutor implements T public ScheduledFuture scheduleAtFixedRate(Runnable task, Instant startTime, Duration period) { Duration initialDelay = Duration.between(this.clock.instant(), startTime); try { - return this.scheduledExecutor.scheduleAtFixedRate(decorateTask(task, true), initialDelay.toNanos(), period.toNanos(), TimeUnit.NANOSECONDS); + return this.scheduledExecutor.scheduleAtFixedRate(decorateTask(task, true), + NANO.convert(initialDelay), NANO.convert(period), NANO); } catch (RejectedExecutionException ex) { throw new TaskRejectedException("Executor [" + this.scheduledExecutor + "] did not accept task: " + task, ex); @@ -232,7 +237,8 @@ public class ConcurrentTaskScheduler extends ConcurrentTaskExecutor implements T @Override public ScheduledFuture scheduleAtFixedRate(Runnable task, Duration period) { try { - return this.scheduledExecutor.scheduleAtFixedRate(decorateTask(task, true), 0, period.toNanos(), TimeUnit.NANOSECONDS); + return this.scheduledExecutor.scheduleAtFixedRate(decorateTask(task, true), + 0, NANO.convert(period), NANO); } catch (RejectedExecutionException ex) { throw new TaskRejectedException("Executor [" + this.scheduledExecutor + "] did not accept task: " + task, ex); @@ -243,7 +249,8 @@ public class ConcurrentTaskScheduler extends ConcurrentTaskExecutor implements T public ScheduledFuture scheduleWithFixedDelay(Runnable task, Instant startTime, Duration delay) { Duration initialDelay = Duration.between(this.clock.instant(), startTime); try { - return this.scheduledExecutor.scheduleWithFixedDelay(decorateTask(task, true), initialDelay.toNanos(), delay.toNanos(), TimeUnit.NANOSECONDS); + return this.scheduledExecutor.scheduleWithFixedDelay(decorateTask(task, true), + NANO.convert(initialDelay), NANO.convert(delay), NANO); } catch (RejectedExecutionException ex) { throw new TaskRejectedException("Executor [" + this.scheduledExecutor + "] did not accept task: " + task, ex); @@ -253,7 +260,8 @@ public class ConcurrentTaskScheduler extends ConcurrentTaskExecutor implements T @Override public ScheduledFuture scheduleWithFixedDelay(Runnable task, Duration delay) { try { - return this.scheduledExecutor.scheduleWithFixedDelay(decorateTask(task, true), 0, delay.toNanos(), TimeUnit.NANOSECONDS); + return this.scheduledExecutor.scheduleWithFixedDelay(decorateTask(task, true), + 0, NANO.convert(delay), NANO); } catch (RejectedExecutionException ex) { throw new TaskRejectedException("Executor [" + this.scheduledExecutor + "] did not accept task: " + task, ex); diff --git a/spring-context/src/main/java/org/springframework/scheduling/concurrent/ThreadPoolTaskScheduler.java b/spring-context/src/main/java/org/springframework/scheduling/concurrent/ThreadPoolTaskScheduler.java index 07a8b9855a7..bffdc53bef8 100644 --- a/spring-context/src/main/java/org/springframework/scheduling/concurrent/ThreadPoolTaskScheduler.java +++ b/spring-context/src/main/java/org/springframework/scheduling/concurrent/ThreadPoolTaskScheduler.java @@ -63,6 +63,9 @@ import org.springframework.util.concurrent.ListenableFutureTask; public class ThreadPoolTaskScheduler extends ExecutorConfigurationSupport implements AsyncListenableTaskExecutor, SchedulingTaskExecutor, TaskScheduler { + private static final TimeUnit NANO = TimeUnit.NANOSECONDS; + + private volatile int poolSize = 1; private volatile boolean removeOnCancelPolicy; @@ -382,7 +385,8 @@ public class ThreadPoolTaskScheduler extends ExecutorConfigurationSupport ScheduledExecutorService executor = getScheduledExecutor(); Duration initialDelay = Duration.between(this.clock.instant(), startTime); try { - return executor.schedule(errorHandlingTask(task, false), initialDelay.toNanos(), TimeUnit.NANOSECONDS); + return executor.schedule(errorHandlingTask(task, false), + NANO.convert(initialDelay), NANO); } catch (RejectedExecutionException ex) { throw new TaskRejectedException("Executor [" + executor + "] did not accept task: " + task, ex); @@ -394,7 +398,8 @@ public class ThreadPoolTaskScheduler extends ExecutorConfigurationSupport ScheduledExecutorService executor = getScheduledExecutor(); Duration initialDelay = Duration.between(this.clock.instant(), startTime); try { - return executor.scheduleAtFixedRate(errorHandlingTask(task, true), initialDelay.toNanos(), period.toNanos(), TimeUnit.NANOSECONDS); + return executor.scheduleAtFixedRate(errorHandlingTask(task, true), + NANO.convert(initialDelay), NANO.convert(period), NANO); } catch (RejectedExecutionException ex) { throw new TaskRejectedException("Executor [" + executor + "] did not accept task: " + task, ex); @@ -405,7 +410,8 @@ public class ThreadPoolTaskScheduler extends ExecutorConfigurationSupport public ScheduledFuture scheduleAtFixedRate(Runnable task, Duration period) { ScheduledExecutorService executor = getScheduledExecutor(); try { - return executor.scheduleAtFixedRate(errorHandlingTask(task, true), 0, period.toNanos(), TimeUnit.NANOSECONDS); + return executor.scheduleAtFixedRate(errorHandlingTask(task, true), + 0, NANO.convert(period), NANO); } catch (RejectedExecutionException ex) { throw new TaskRejectedException("Executor [" + executor + "] did not accept task: " + task, ex); @@ -417,7 +423,8 @@ public class ThreadPoolTaskScheduler extends ExecutorConfigurationSupport ScheduledExecutorService executor = getScheduledExecutor(); Duration initialDelay = Duration.between(this.clock.instant(), startTime); try { - return executor.scheduleWithFixedDelay(errorHandlingTask(task, true), initialDelay.toNanos(), delay.toNanos(), TimeUnit.NANOSECONDS); + return executor.scheduleWithFixedDelay(errorHandlingTask(task, true), + NANO.convert(initialDelay), NANO.convert(delay), NANO); } catch (RejectedExecutionException ex) { throw new TaskRejectedException("Executor [" + executor + "] did not accept task: " + task, ex); @@ -428,7 +435,8 @@ public class ThreadPoolTaskScheduler extends ExecutorConfigurationSupport public ScheduledFuture scheduleWithFixedDelay(Runnable task, Duration delay) { ScheduledExecutorService executor = getScheduledExecutor(); try { - return executor.scheduleWithFixedDelay(errorHandlingTask(task, true), 0, delay.toNanos(), TimeUnit.NANOSECONDS); + return executor.scheduleWithFixedDelay(errorHandlingTask(task, true), + 0, NANO.convert(delay), NANO); } catch (RejectedExecutionException ex) { throw new TaskRejectedException("Executor [" + executor + "] did not accept task: " + task, ex); diff --git a/spring-context/src/test/java/org/springframework/scheduling/annotation/ScheduledAnnotationBeanPostProcessorTests.java b/spring-context/src/test/java/org/springframework/scheduling/annotation/ScheduledAnnotationBeanPostProcessorTests.java index a1cd409d205..417de9d8255 100644 --- a/spring-context/src/test/java/org/springframework/scheduling/annotation/ScheduledAnnotationBeanPostProcessorTests.java +++ b/spring-context/src/test/java/org/springframework/scheduling/annotation/ScheduledAnnotationBeanPostProcessorTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2022 the original author or authors. + * 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. @@ -95,6 +95,7 @@ class ScheduledAnnotationBeanPostProcessorTests { FixedDelay, 5_000 FixedDelayInSeconds, 5_000 FixedDelayInMinutes, 180_000 + FixedDelayWithMaxValue, -1 """) void fixedDelayTask(@NameToClass Class beanClass, long expectedInterval) { BeanDefinition processorDefinition = new RootBeanDefinition(ScheduledAnnotationBeanPostProcessor.class); @@ -120,7 +121,8 @@ class ScheduledAnnotationBeanPostProcessorTests { assertThat(targetObject).isEqualTo(target); assertThat(targetMethod.getName()).isEqualTo("fixedDelay"); assertThat(task.getInitialDelayDuration()).isZero(); - assertThat(task.getIntervalDuration()).isEqualTo(Duration.ofMillis(expectedInterval)); + assertThat(task.getIntervalDuration()).isEqualTo( + Duration.ofMillis(expectedInterval < 0 ? Long.MAX_VALUE : expectedInterval)); } @ParameterizedTest @@ -343,8 +345,7 @@ class ScheduledAnnotationBeanPostProcessorTests { BeanDefinition targetDefinition = new RootBeanDefinition(CronWithInvalidTimezoneTestBean.class); context.registerBeanDefinition("postProcessor", processorDefinition); context.registerBeanDefinition("target", targetDefinition); - assertThatExceptionOfType(BeanCreationException.class).isThrownBy( - context::refresh); + assertThatExceptionOfType(BeanCreationException.class).isThrownBy(context::refresh); } @Test @@ -355,8 +356,7 @@ class ScheduledAnnotationBeanPostProcessorTests { context.registerBeanDefinition("methodValidation", validationDefinition); context.registerBeanDefinition("postProcessor", processorDefinition); context.registerBeanDefinition("target", targetDefinition); - assertThatExceptionOfType(BeanCreationException.class).isThrownBy( - context::refresh); + assertThatExceptionOfType(BeanCreationException.class).isThrownBy(context::refresh); } @Test @@ -702,18 +702,16 @@ class ScheduledAnnotationBeanPostProcessorTests { BeanDefinition targetDefinition = new RootBeanDefinition(EmptyAnnotationTestBean.class); context.registerBeanDefinition("postProcessor", processorDefinition); context.registerBeanDefinition("target", targetDefinition); - assertThatExceptionOfType(BeanCreationException.class).isThrownBy( - context::refresh); + assertThatExceptionOfType(BeanCreationException.class).isThrownBy(context::refresh); } @Test - void invalidCron() throws Throwable { + void invalidCron() { BeanDefinition processorDefinition = new RootBeanDefinition(ScheduledAnnotationBeanPostProcessor.class); BeanDefinition targetDefinition = new RootBeanDefinition(InvalidCronTestBean.class); context.registerBeanDefinition("postProcessor", processorDefinition); context.registerBeanDefinition("target", targetDefinition); - assertThatExceptionOfType(BeanCreationException.class).isThrownBy( - context::refresh); + assertThatExceptionOfType(BeanCreationException.class).isThrownBy(context::refresh); } @Test @@ -722,8 +720,7 @@ class ScheduledAnnotationBeanPostProcessorTests { BeanDefinition targetDefinition = new RootBeanDefinition(NonEmptyParamListTestBean.class); context.registerBeanDefinition("postProcessor", processorDefinition); context.registerBeanDefinition("target", targetDefinition); - assertThatExceptionOfType(BeanCreationException.class).isThrownBy( - context::refresh); + assertThatExceptionOfType(BeanCreationException.class).isThrownBy(context::refresh); } @@ -748,6 +745,13 @@ class ScheduledAnnotationBeanPostProcessorTests { } } + static class FixedDelayWithMaxValue { + + @Scheduled(fixedDelay = Long.MAX_VALUE) + void fixedDelay() { + } + } + static class FixedRate { From b77d4d01c5fe30e8d8a5e40293bcd71e1841fa7b Mon Sep 17 00:00:00 2001 From: Juergen Hoeller Date: Mon, 26 Jun 2023 19:28:19 +0200 Subject: [PATCH 2/3] Adapt no-arg value from interface-based InvocationHandler callback Closes gh-30756 --- .../annotation/MvcUriComponentsBuilder.java | 4 +- .../MvcUriComponentsBuilderTests.java | 46 +++++++++++++++++-- 2 files changed, 45 insertions(+), 5 deletions(-) diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/MvcUriComponentsBuilder.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/MvcUriComponentsBuilder.java index 5086cf055e7..839b51d51d3 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/MvcUriComponentsBuilder.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/MvcUriComponentsBuilder.java @@ -745,8 +745,8 @@ public class MvcUriComponentsBuilder { @Override @Nullable - public Object invoke(Object proxy, Method method, Object[] args) throws Throwable { - return intercept(proxy, method, args, null); + public Object invoke(Object proxy, Method method, @Nullable Object[] args) { + return intercept(proxy, method, (args != null ? args : new Object[0]), null); } @Override diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/MvcUriComponentsBuilderTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/MvcUriComponentsBuilderTests.java index 4e988c32400..daab23fab35 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/MvcUriComponentsBuilderTests.java +++ b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/MvcUriComponentsBuilderTests.java @@ -293,6 +293,14 @@ public class MvcUriComponentsBuilderTests { assertThat(uriComponents.toUriString()).isEqualTo("http://localhost/input"); } + @Test + public void fromMethodCallOnSubclass() { + UriComponents uriComponents = fromMethodCall(on(ExtendedController.class).myMethod(null)).build(); + + assertThat(uriComponents.toUriString()).startsWith("http://localhost"); + assertThat(uriComponents.toUriString()).endsWith("/extended/else"); + } + @Test public void fromMethodCallPlain() { UriComponents uriComponents = fromMethodCall(on(ControllerWithMethods.class).myMethod(null)).build(); @@ -302,11 +310,27 @@ public class MvcUriComponentsBuilderTests { } @Test - public void fromMethodCallOnSubclass() { - UriComponents uriComponents = fromMethodCall(on(ExtendedController.class).myMethod(null)).build(); + public void fromMethodCallPlainWithNoArguments() { + UriComponents uriComponents = fromMethodCall(on(ControllerWithMethods.class).myMethod()).build(); assertThat(uriComponents.toUriString()).startsWith("http://localhost"); - assertThat(uriComponents.toUriString()).endsWith("/extended/else"); + assertThat(uriComponents.toUriString()).endsWith("/something/noarg"); + } + + @Test + public void fromMethodCallPlainOnInterface() { + UriComponents uriComponents = fromMethodCall(on(ControllerInterface.class).myMethod(null)).build(); + + assertThat(uriComponents.toUriString()).startsWith("http://localhost"); + assertThat(uriComponents.toUriString()).endsWith("/something/else"); + } + + @Test + public void fromMethodCallPlainWithNoArgumentsOnInterface() { + UriComponents uriComponents = fromMethodCall(on(ControllerInterface.class).myMethod()).build(); + + assertThat(uriComponents.toUriString()).startsWith("http://localhost"); + assertThat(uriComponents.toUriString()).endsWith("/something/noarg"); } @Test @@ -575,6 +599,11 @@ public class MvcUriComponentsBuilderTests { return null; } + @RequestMapping("/noarg") + HttpEntity myMethod() { + return null; + } + @RequestMapping("/{id}/foo") HttpEntity methodWithPathVariable(@PathVariable String id) { return null; @@ -616,6 +645,17 @@ public class MvcUriComponentsBuilderTests { } + @RequestMapping("/something") + public interface ControllerInterface { + + @RequestMapping("/else") + HttpEntity myMethod(@RequestBody Object payload); + + @RequestMapping("/noarg") + HttpEntity myMethod(); + } + + @RequestMapping("/user/{userId}/contacts") static class UserContactController { From 6526e79eea450e49558035fc22b1c8d11ef2c285 Mon Sep 17 00:00:00 2001 From: Juergen Hoeller Date: Mon, 26 Jun 2023 19:28:38 +0200 Subject: [PATCH 3/3] Polishing --- .../messaging/handler/invocation/ResolvableMethod.java | 6 ++++-- .../web/testfixture/method/ResolvableMethod.java | 6 ++++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/spring-messaging/src/test/java/org/springframework/messaging/handler/invocation/ResolvableMethod.java b/spring-messaging/src/test/java/org/springframework/messaging/handler/invocation/ResolvableMethod.java index e32352679b3..31df4ee8cc6 100644 --- a/spring-messaging/src/test/java/org/springframework/messaging/handler/invocation/ResolvableMethod.java +++ b/spring-messaging/src/test/java/org/springframework/messaging/handler/invocation/ResolvableMethod.java @@ -613,11 +613,12 @@ public class ResolvableMethod { private static class MethodInvocationInterceptor implements MethodInterceptor, InvocationHandler { + @Nullable private Method invokedMethod; @Override @Nullable - public Object intercept(Object object, Method method, Object[] args, MethodProxy proxy) { + public Object intercept(Object object, Method method, @Nullable Object[] args, @Nullable MethodProxy proxy) { if (ReflectionUtils.isObjectMethod(method)) { return ReflectionUtils.invokeMethod(method, object, args); } @@ -629,10 +630,11 @@ public class ResolvableMethod { @Override @Nullable - public Object invoke(Object proxy, Method method, Object[] args) throws Throwable { + public Object invoke(Object proxy, Method method, @Nullable Object[] args) { return intercept(proxy, method, args, null); } + @Nullable Method getInvokedMethod() { return this.invokedMethod; } diff --git a/spring-web/src/testFixtures/java/org/springframework/web/testfixture/method/ResolvableMethod.java b/spring-web/src/testFixtures/java/org/springframework/web/testfixture/method/ResolvableMethod.java index cb6d34f3e77..abd6b2c0510 100644 --- a/spring-web/src/testFixtures/java/org/springframework/web/testfixture/method/ResolvableMethod.java +++ b/spring-web/src/testFixtures/java/org/springframework/web/testfixture/method/ResolvableMethod.java @@ -617,11 +617,12 @@ public class ResolvableMethod { private static class MethodInvocationInterceptor implements MethodInterceptor, InvocationHandler { + @Nullable private Method invokedMethod; @Override @Nullable - public Object intercept(Object object, Method method, Object[] args, MethodProxy proxy) { + public Object intercept(Object object, Method method, @Nullable Object[] args, @Nullable MethodProxy proxy) { if (ReflectionUtils.isObjectMethod(method)) { return ReflectionUtils.invokeMethod(method, object, args); } @@ -633,10 +634,11 @@ public class ResolvableMethod { @Override @Nullable - public Object invoke(Object proxy, Method method, Object[] args) throws Throwable { + public Object invoke(Object proxy, Method method, @Nullable Object[] args) { return intercept(proxy, method, args, null); } + @Nullable Method getInvokedMethod() { return this.invokedMethod; }