Allow to schedule tasks up to Long.MAX_VALUE (#7972)

Motivation: We should allow to schedule tasks with a delay up to Long.MAX_VALUE as we did pre 4.1.25.Final. Modifications: Just ensure we not overflow and put the correct max limits in place when schedule a timer. At worse we will get a wakeup to early and then schedule a new timeout. Result: Fixes #7970.
netty · May 30, 2018 · d133bf0 · d133bf0
1 parent ec91c40
commit d133bf0
Show file tree

Hide file tree

Showing 9 changed files with 52 additions and 106 deletions.
diff --git a/common/src/main/java/io/netty/util/concurrent/AbstractScheduledEventExecutor.java b/common/src/main/java/io/netty/util/concurrent/AbstractScheduledEventExecutor.java
@@ -150,7 +150,7 @@ public ScheduledFuture<?> schedule(Runnable command, long delay, TimeUnit unit)
         if (delay < 0) {
             delay = 0;
         }
-        validateScheduled(delay, unit);
+        validateScheduled0(delay, unit);
 
         return schedule(new ScheduledFutureTask<Void>(
                 this, command, null, ScheduledFutureTask.deadlineNanos(unit.toNanos(delay))));
@@ -163,7 +163,7 @@ public <V> ScheduledFuture<V> schedule(Callable<V> callable, long delay, TimeUni
         if (delay < 0) {
             delay = 0;
         }
-        validateScheduled(delay, unit);
+        validateScheduled0(delay, unit);
 
         return schedule(new ScheduledFutureTask<V>(
                 this, callable, ScheduledFutureTask.deadlineNanos(unit.toNanos(delay))));
@@ -181,8 +181,8 @@ public ScheduledFuture<?> scheduleAtFixedRate(Runnable command, long initialDela
             throw new IllegalArgumentException(
                     String.format("period: %d (expected: > 0)", period));
         }
-        validateScheduled(initialDelay, unit);
-        validateScheduled(period, unit);
+        validateScheduled0(initialDelay, unit);
+        validateScheduled0(period, unit);
 
         return schedule(new ScheduledFutureTask<Void>(
                 this, Executors.<Void>callable(command, null),
@@ -202,17 +202,25 @@ public ScheduledFuture<?> scheduleWithFixedDelay(Runnable command, long initialD
                     String.format("delay: %d (expected: > 0)", delay));
         }
 
-        validateScheduled(initialDelay, unit);
-        validateScheduled(delay, unit);
+        validateScheduled0(initialDelay, unit);
+        validateScheduled0(delay, unit);
 
         return schedule(new ScheduledFutureTask<Void>(
                 this, Executors.<Void>callable(command, null),
                 ScheduledFutureTask.deadlineNanos(unit.toNanos(initialDelay)), -unit.toNanos(delay)));
     }
 
+    @SuppressWarnings("deprecation")
+    private void validateScheduled0(long amount, TimeUnit unit) {
+        validateScheduled(amount, unit);
+    }
+
     /**
      * Sub-classes may override this to restrict the maximal amount of time someone can use to schedule a task.
+     *
+     * @deprecated will be removed in the future.
      */
+    @Deprecated
     protected void validateScheduled(long amount, TimeUnit unit) {
         // NOOP
     }

diff --git a/common/src/main/java/io/netty/util/concurrent/ScheduledFutureTask.java b/common/src/main/java/io/netty/util/concurrent/ScheduledFutureTask.java
@@ -35,7 +35,9 @@ static long nanoTime() {
     }
 
     static long deadlineNanos(long delay) {
-        return nanoTime() + delay;
+        long deadlineNanos = nanoTime() + delay;
+        // Guard against overflow
+        return deadlineNanos < 0 ? Long.MAX_VALUE : deadlineNanos;
     }
 
     private final long id = nextTaskId.getAndIncrement();

diff --git a/common/src/test/java/io/netty/util/concurrent/ScheduledFutureTaskTest.java b/common/src/test/java/io/netty/util/concurrent/ScheduledFutureTaskTest.java
@@ -0,0 +1,27 @@
+/*
+ * Copyright 2018 The Netty Project
+ *
+ * The Netty Project licenses this file to you 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:
+ *
+ *   http://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 io.netty.util.concurrent;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+public class ScheduledFutureTaskTest {
+
+    @Test
+    public void testDeadlineNanosNotOverflow() {
+        Assert.assertEquals(Long.MAX_VALUE, ScheduledFutureTask.deadlineNanos(Long.MAX_VALUE));
+    }
+}
diff --git a/transport-native-epoll/src/main/java/io/netty/channel/epoll/EpollEventLoop.java b/transport-native-epoll/src/main/java/io/netty/channel/epoll/EpollEventLoop.java
@@ -37,7 +37,6 @@
 import java.util.Queue;
 import java.util.concurrent.Callable;
 import java.util.concurrent.Executor;
-import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicIntegerFieldUpdater;
 
 import static java.lang.Math.min;
@@ -80,7 +79,7 @@ public Integer call() throws Exception {
     private volatile int ioRatio = 50;
 
     // See http://man7.org/linux/man-pages/man2/timerfd_create.2.html.
-    static final long MAX_SCHEDULED_DAYS = TimeUnit.SECONDS.toDays(999999999);
+    private static final long MAX_SCHEDULED_TIMERFD_NS = 999999999;
 
     EpollEventLoop(EventLoopGroup parent, Executor executor, int maxEvents,
                    SelectStrategy strategy, RejectedExecutionHandler rejectedExecutionHandler) {
@@ -237,7 +236,7 @@ private int epollWait(boolean oldWakeup) throws IOException {
         long totalDelay = delayNanos(System.nanoTime());
         int delaySeconds = (int) min(totalDelay / 1000000000L, Integer.MAX_VALUE);
         return Native.epollWait(epollFd, events, timerFd, delaySeconds,
-                (int) min(totalDelay - delaySeconds * 1000000000L, Integer.MAX_VALUE));
+                (int) min(MAX_SCHEDULED_TIMERFD_NS, totalDelay - delaySeconds * 1000000000L));
     }
 
     private int epollWaitNow() throws IOException {
@@ -453,12 +452,4 @@ protected void cleanup() {
             events.free();
         }
     }
-
-    @Override
-    protected void validateScheduled(long amount, TimeUnit unit) {
-        long days = unit.toDays(amount);
-        if (days > MAX_SCHEDULED_DAYS) {
-            throw new IllegalArgumentException("days: " + days + " (expected: < " + MAX_SCHEDULED_DAYS + ')');
-        }
-    }
 }
diff --git a/transport-native-epoll/src/test/java/io/netty/channel/epoll/EpollEventLoopTest.java b/transport-native-epoll/src/test/java/io/netty/channel/epoll/EpollEventLoopTest.java
@@ -24,32 +24,11 @@
 
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
-import static org.junit.Assert.fail;
 
 public class EpollEventLoopTest {
 
-    @Test(timeout = 5000L)
-    public void testScheduleBigDelayOverMax() {
-        EventLoopGroup group = new EpollEventLoopGroup(1);
-
-        final EventLoop el = group.next();
-        try {
-            el.schedule(new Runnable() {
-                @Override
-                public void run() {
-                    // NOOP
-                }
-            }, Integer.MAX_VALUE, TimeUnit.DAYS);
-            fail();
-        } catch (IllegalArgumentException expected) {
-            // expected
-        }
-
-        group.shutdownGracefully();
-    }
-
     @Test
-    public void testScheduleBigDelay() {
+    public void testScheduleBigDelayNotOverflow() {
         EventLoopGroup group = new EpollEventLoopGroup(1);
 
         final EventLoop el = group.next();
@@ -58,7 +37,7 @@ public void testScheduleBigDelay() {
             public void run() {
                 // NOOP
             }
-        }, EpollEventLoop.MAX_SCHEDULED_DAYS, TimeUnit.DAYS);
+        }, Long.MAX_VALUE, TimeUnit.MILLISECONDS);
 
         assertFalse(future.awaitUninterruptibly(1000));
         assertTrue(future.cancel(true));

diff --git a/transport-native-kqueue/src/main/java/io/netty/channel/kqueue/KQueueEventLoop.java b/transport-native-kqueue/src/main/java/io/netty/channel/kqueue/KQueueEventLoop.java
@@ -33,7 +33,6 @@
 import java.util.Queue;
 import java.util.concurrent.Callable;
 import java.util.concurrent.Executor;
-import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicIntegerFieldUpdater;
 
 import static io.netty.channel.kqueue.KQueueEventArray.deleteGlobalRefs;
@@ -77,8 +76,6 @@ public Integer call() throws Exception {
     private volatile int wakenUp;
     private volatile int ioRatio = 50;
 
-    static final long MAX_SCHEDULED_DAYS = 365 * 3;
-
     KQueueEventLoop(EventLoopGroup parent, Executor executor, int maxEvents,
                     SelectStrategy strategy, RejectedExecutionHandler rejectedExecutionHandler) {
         super(parent, executor, false, DEFAULT_MAX_PENDING_TASKS, rejectedExecutionHandler);
@@ -370,12 +367,4 @@ private static void handleLoopException(Throwable t) {
             // Ignore.
         }
     }
-
-    @Override
-    protected void validateScheduled(long amount, TimeUnit unit) {
-        long days = unit.toDays(amount);
-        if (days > MAX_SCHEDULED_DAYS) {
-            throw new IllegalArgumentException("days: " + days + " (expected: < " + MAX_SCHEDULED_DAYS + ')');
-        }
-    }
 }
diff --git a/transport-native-kqueue/src/test/java/io/netty/channel/kqueue/KQueueEventLoopTest.java b/transport-native-kqueue/src/test/java/io/netty/channel/kqueue/KQueueEventLoopTest.java
@@ -24,32 +24,11 @@
 
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
-import static org.junit.Assert.fail;
 
 public class KQueueEventLoopTest {
 
-    @Test(timeout = 5000L)
-    public void testScheduleBigDelayOverMax() {
-        EventLoopGroup group = new KQueueEventLoopGroup(1);
-
-        final EventLoop el = group.next();
-        try {
-            el.schedule(new Runnable() {
-                @Override
-                public void run() {
-                    // NOOP
-                }
-            }, Integer.MAX_VALUE, TimeUnit.DAYS);
-            fail();
-        } catch (IllegalArgumentException expected) {
-            // expected
-        }
-
-        group.shutdownGracefully();
-    }
-
     @Test
-    public void testScheduleBigDelay() {
+    public void testScheduleBigDelayNotOverflow() {
         EventLoopGroup group = new KQueueEventLoopGroup(1);
 
         final EventLoop el = group.next();
@@ -58,7 +37,7 @@ public void testScheduleBigDelay() {
             public void run() {
                 // NOOP
             }
-        }, KQueueEventLoop.MAX_SCHEDULED_DAYS, TimeUnit.DAYS);
+        }, Long.MAX_VALUE, TimeUnit.MILLISECONDS);
 
         assertFalse(future.awaitUninterruptibly(1000));
         assertTrue(future.cancel(true));

diff --git a/transport/src/main/java/io/netty/channel/nio/NioEventLoop.java b/transport/src/main/java/io/netty/channel/nio/NioEventLoop.java
@@ -113,8 +113,6 @@ public Void run() {
         }
     }
 
-    static final long MAX_SCHEDULED_DAYS = 365 * 3;
-
     /**
      * The NIO {@link Selector}.
      */
@@ -825,12 +823,4 @@ private void selectAgain() {
             logger.warn("Failed to update SelectionKeys.", t);
         }
     }
-
-    @Override
-    protected void validateScheduled(long amount, TimeUnit unit) {
-        long days = unit.toDays(amount);
-        if (days > MAX_SCHEDULED_DAYS) {
-            throw new IllegalArgumentException("days: " + days + " (expected: < " + MAX_SCHEDULED_DAYS + ')');
-        }
-    }
 }
diff --git a/transport/src/test/java/io/netty/channel/nio/NioEventLoopTest.java b/transport/src/test/java/io/netty/channel/nio/NioEventLoopTest.java
@@ -73,27 +73,8 @@ public void run() {
         }
     }
 
-    @Test(timeout = 5000L)
-    public void testScheduleBigDelayOverMax() {
-        EventLoopGroup group = new NioEventLoopGroup(1);
-        final EventLoop el = group.next();
-        try {
-            el.schedule(new Runnable() {
-                @Override
-                public void run() {
-                    // NOOP
-                }
-            }, Integer.MAX_VALUE, TimeUnit.DAYS);
-            fail();
-        } catch (IllegalArgumentException expected) {
-            // expected
-        }
-
-        group.shutdownGracefully();
-    }
-
     @Test
-    public void testScheduleBigDelay() {
+    public void testScheduleBigDelayNotOverflow() {
         EventLoopGroup group = new NioEventLoopGroup(1);
 
         final EventLoop el = group.next();
@@ -102,7 +83,7 @@ public void testScheduleBigDelay() {
             public void run() {
                 // NOOP
             }
-        }, NioEventLoop.MAX_SCHEDULED_DAYS, TimeUnit.DAYS);
+        }, Long.MAX_VALUE, TimeUnit.MILLISECONDS);
 
         assertFalse(future.awaitUninterruptibly(1000));
         assertTrue(future.cancel(true));