Uploaded image for project: 'JDK'
  1. JDK
  2. JDK-8171816

Design troubles with Delayed and ScheduledThreadPoolExecutor

    Details

    • Type: Bug
    • Status: Open
    • Priority: P3
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: core-libs
    • Labels:
      None

      Description

      Delayed implements Comparable<Delayed> but there is no way for a general Delayed implementation to fulfill the Comparable contract.

      A Delayed implementation has no choice but to compare
      this.getDelay() with other.getDelay()
      but because time keeps slipping into the future, two Delayed with exactly the same expiration time may compare differently, and compareTo is not necessarily anti-symmetric. It's fundamentally flaky.

      As a result, ScheduledThreadPoolExecutor cannot really fulfill its promise """Tasks scheduled for exactly the same execution time are enabled in first-in-first-out (FIFO) order of submission.""" The promise is problematic in any case because there is no way for a task submitter to specify the precise expiration time. But the implementation fails the weaker promise that a sequence of tasks with identical delays are scheduled in FIFO order.

      This affects users of STPE who use decorateTask and depend on the tasks being scheduled in FIFO order. The workaround is for decorateTask to retain a reference to the underlying ScheduledFutureTask and delegate to the compareTo method on that. (Any particular concrete implementation will be safe by simply comparing trigger times)

      Another problem is that getDelay() must call nanoTime to compute the delay, and this is unnecessarily expensive.

        Activity

        Hide
        martin Martin Buchholz added a comment -
        In practice, subclasses of ScheduledThreadPoolExecutor probably only use decorateTask with one particular Delayed implementation. Such subclasses can get the same FIFO properties as ScheduledFutureTask by following the pattern:

            static class CustomTask<V> implements RunnableScheduledFuture<V> {
                private final RunnableScheduledFuture<V> task;
                CustomTask(RunnableScheduledFuture<V> task) { this.task = task; }
                public long getDelay(TimeUnit unit) { return task.getDelay(unit); }
                public int compareTo(Delayed t) {
                    return task.compareTo(((CustomTask)t).task);
                }

        We should at least probably expand the sample code in STPE javadoc to use this idiom.
        Show
        martin Martin Buchholz added a comment - In practice, subclasses of ScheduledThreadPoolExecutor probably only use decorateTask with one particular Delayed implementation. Such subclasses can get the same FIFO properties as ScheduledFutureTask by following the pattern:     static class CustomTask<V> implements RunnableScheduledFuture<V> {         private final RunnableScheduledFuture<V> task;         CustomTask(RunnableScheduledFuture<V> task) { this.task = task; }         public long getDelay(TimeUnit unit) { return task.getDelay(unit); }         public int compareTo(Delayed t) {             return task.compareTo(((CustomTask)t).task);         } We should at least probably expand the sample code in STPE javadoc to use this idiom.
        Hide
        martin Martin Buchholz added a comment -
        It's not going to be easy to make Delayed a well-behaved interface.

        We could add default methods like

        /* nanoTime based */ default long triggerTime() { return System.nanoTime() + getDelay(); }

        and then have all concrete implementations override triggerTime in the obvious efficient way, but ...
        even then we'll have trouble with overflow, that STPE handles by constraining all values to within Long.MAX_VALUE of each other. If we insisted on a max past horizon of 0 (as STPE does) and max future horizon of MAX_VALUE/2 at creation time, then things would work well, especially if expired Delayeds are "promptly" discarded.

        default int compareTo(Delayed o) { return triggerTime() - o.triggerTime(); }
        Show
        martin Martin Buchholz added a comment - It's not going to be easy to make Delayed a well-behaved interface. We could add default methods like /* nanoTime based */ default long triggerTime() { return System.nanoTime() + getDelay(); } and then have all concrete implementations override triggerTime in the obvious efficient way, but ... even then we'll have trouble with overflow, that STPE handles by constraining all values to within Long.MAX_VALUE of each other. If we insisted on a max past horizon of 0 (as STPE does) and max future horizon of MAX_VALUE/2 at creation time, then things would work well, especially if expired Delayeds are "promptly" discarded. default int compareTo(Delayed o) { return triggerTime() - o.triggerTime(); }

          People

          • Assignee:
            martin Martin Buchholz
            Reporter:
            martin Martin Buchholz
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated: