[concurrency-interest] Adding toString methods for task objects?

Martin Buchholz martinrb at google.com
Wed Aug 9 14:20:26 EDT 2017


Josh says, """Item 10: Always override toString"""

toString says """“a concise but informative representation that is easy for
a person to read”""

which also suggests we don't allow unbounded recursion calling toString of
result objects.

I don't think it's a spec change.  Yes, readers of the javadoc can see that
e.g. FutureTask does not currently override Object.toString(), but I don't
think that's a promise to not do so in a future release.  Tightening the
spec of public classes might push existing subclasses out of compliance, so
maybe use @implSpec.

There does not seem to be a common idiom of adding a debugString method to
core library classes.
In any case, one can only do this with public classes like FutureTask, not
hidden ones.

For adapter wrappers like RunnableAdapter that simply convert e.g. Runnable
to Callable, it seems very reasonable for toString() to *also* "wrap" the
toString() of the adapted Runnable.

I'm generally supportive of doing something here, as long as we avoid
calling toString on unknown result objects - OTOH calling toString on
nested task objects seems alright.


On Tue, Aug 8, 2017 at 6:58 PM, Joe Bowbeer <joe.bowbeer at gmail.com> wrote:

> ​I would consider this to be an API change. For example, according to the
> current javadoc, FutureTask's toString method is inherited from Object.
>
> Practically speaking, I'd be concerned about unexpected and unwanted
> changes to the output/log statements of programs that have up until now
> been working fine. In addition to generating a lot of output that may be
> unwanted, this may also generate exceptions. For example,
> PrivilegedCallable.toString calls the toString method of the wrapped
> Callable, which may never have been called previously (bam!), and therefore
> might throw an exception. Or, the toString method of the wrapped callable
> may have been designed to be called in a different context.
>
> If any of the state the toString implementations are accessing is not
> thread-safe, I'd also be concerned about that.
>
> If tasks need toString implementations, I prefer to write them.​
>
> However, I think it would be OK to introduce new convenience methods:
> debugString()
>
> On Tue, Aug 8, 2017 at 6:36 PM, Martin Buchholz <martinrb at google.com>
> wrote:
>
>> My colleague Charles Munger suggests adding toString methods to various
>> task classes in j.u.c.  Obviously this makes it easier to debug large
>> programs that have gone wrong.  But it obviously also is a behavior change
>> that has the potential for trouble, even when everyone is behaving nicely,
>> since toString may have the effect of generating an unbounded output.  I
>> almost started down this road myself a few years ago but chickened out.
>> What do you think?
>>
>> Index: Executors.java
>> ===================================================================
>> RCS file: /export/home/jsr166/jsr166/jsr166/src/main/java/util/concurr
>> ent/Executors.java,v
>> retrieving revision 1.97
>> diff -u -U 10 -r1.97 Executors.java
>> --- Executors.java 15 Apr 2017 00:12:38 -0000 1.97
>> +++ Executors.java 9 Aug 2017 01:34:25 -0000
>> @@ -478,20 +478,24 @@
>>          private final Runnable task;
>>          private final T result;
>>          RunnableAdapter(Runnable task, T result) {
>>              this.task = task;
>>              this.result = result;
>>          }
>>          public T call() {
>>              task.run();
>>              return result;
>>          }
>> +        public String toString() {
>> +            return super.toString()
>> +                + "[task=[" + task + "], result=[" + result + "]]";
>> +        }
>>      }
>>
>>      /**
>>       * A callable that runs under established access control settings.
>>       */
>>      private static final class PrivilegedCallable<T> implements
>> Callable<T> {
>>          final Callable<T> task;
>>          final AccessControlContext acc;
>>
>>          PrivilegedCallable(Callable<T> task) {
>> @@ -504,20 +508,24 @@
>>                  return AccessController.doPrivileged(
>>                      new PrivilegedExceptionAction<T>() {
>>                          public T run() throws Exception {
>>                              return task.call();
>>                          }
>>                      }, acc);
>>              } catch (PrivilegedActionException e) {
>>                  throw e.getException();
>>              }
>>          }
>> +
>> +        public String toString() {
>> +            return super.toString() + "[task=[" + task + "]]";
>> +        }
>>      }
>>
>>      /**
>>       * A callable that runs under established access control settings and
>>       * current ClassLoader.
>>       */
>>      private static final class PrivilegedCallableUsingCurrent
>> ClassLoader<T>
>>              implements Callable<T> {
>>          final Callable<T> task;
>>          final AccessControlContext acc;
>> @@ -556,20 +564,24 @@
>>                                  } finally {
>>                                      t.setContextClassLoader(cl);
>>                                  }
>>                              }
>>                          }
>>                      }, acc);
>>              } catch (PrivilegedActionException e) {
>>                  throw e.getException();
>>              }
>>          }
>> +
>> +        public String toString() {
>> +            return super.toString() + "[task=[" + task + "]]";
>> +        }
>>      }
>>
>>      /**
>>       * The default thread factory.
>>       */
>>      private static class DefaultThreadFactory implements ThreadFactory {
>>          private static final AtomicInteger poolNumber = new
>> AtomicInteger(1);
>>          private final ThreadGroup group;
>>          private final AtomicInteger threadNumber = new AtomicInteger(1);
>>          private final String namePrefix;
>> Index: ForkJoinTask.java
>> ===================================================================
>> RCS file: /export/home/jsr166/jsr166/jsr166/src/main/java/util/concurr
>> ent/ForkJoinTask.java,v
>> retrieving revision 1.115
>> diff -u -U 10 -r1.115 ForkJoinTask.java
>> --- ForkJoinTask.java 19 Apr 2017 23:45:51 -0000 1.115
>> +++ ForkJoinTask.java 9 Aug 2017 01:34:25 -0000
>> @@ -1339,37 +1339,43 @@
>>          T result;
>>          AdaptedRunnable(Runnable runnable, T result) {
>>              if (runnable == null) throw new NullPointerException();
>>              this.runnable = runnable;
>>              this.result = result; // OK to set this even before
>> completion
>>          }
>>          public final T getRawResult() { return result; }
>>          public final void setRawResult(T v) { result = v; }
>>          public final boolean exec() { runnable.run(); return true; }
>>          public final void run() { invoke(); }
>> +        public String toString() {
>> +            return super.toString() + "[task=[" + runnable + "],
>> result=[" + result + "]]";
>> +        }
>>          private static final long serialVersionUID =
>> 5232453952276885070L;
>>      }
>>
>>      /**
>>       * Adapter for Runnables without results.
>>       */
>>      static final class AdaptedRunnableAction extends ForkJoinTask<Void>
>>          implements RunnableFuture<Void> {
>>          final Runnable runnable;
>>          AdaptedRunnableAction(Runnable runnable) {
>>              if (runnable == null) throw new NullPointerException();
>>              this.runnable = runnable;
>>          }
>>          public final Void getRawResult() { return null; }
>>          public final void setRawResult(Void v) { }
>>          public final boolean exec() { runnable.run(); return true; }
>>          public final void run() { invoke(); }
>> +        public String toString() {
>> +            return super.toString() + "[task=[" + runnable + "]]";
>> +        }
>>          private static final long serialVersionUID =
>> 5232453952276885070L;
>>      }
>>
>>      /**
>>       * Adapter for Runnables in which failure forces worker exception.
>>       */
>>      static final class RunnableExecuteAction extends ForkJoinTask<Void> {
>>          final Runnable runnable;
>>          RunnableExecuteAction(Runnable runnable) {
>>              if (runnable == null) throw new NullPointerException();
>> @@ -1402,20 +1408,23 @@
>>                  result = callable.call();
>>                  return true;
>>              } catch (RuntimeException rex) {
>>                  throw rex;
>>              } catch (Exception ex) {
>>                  throw new RuntimeException(ex);
>>              }
>>          }
>>          public final void run() { invoke(); }
>>          private static final long serialVersionUID =
>> 2838392045355241008L;
>> +        public String toString() {
>> +            return super.toString() + "[task=[" + callable + "]]";
>> +        }
>>      }
>>
>>      /**
>>       * Returns a new {@code ForkJoinTask} that performs the {@code run}
>>       * method of the given {@code Runnable} as its action, and returns
>>       * a null result upon {@link #join}.
>>       *
>>       * @param runnable the runnable action
>>       * @return the task
>>       */
>> Index: FutureTask.java
>> ===================================================================
>> RCS file: /export/home/jsr166/jsr166/jsr166/src/main/java/util/concurr
>> ent/FutureTask.java,v
>> retrieving revision 1.118
>> diff -u -U 10 -r1.118 FutureTask.java
>> --- FutureTask.java 10 Sep 2016 04:06:51 -0000 1.118
>> +++ FutureTask.java 9 Aug 2017 01:34:25 -0000
>> @@ -444,20 +444,49 @@
>>                              continue retry;
>>                      }
>>                      else if (!WAITERS.compareAndSet(this, q, s))
>>                          continue retry;
>>                  }
>>                  break;
>>              }
>>          }
>>      }
>>
>> +    public String toString() {
>> +        String start = super.toString() + "[status=";
>> +        try {
>> +            switch (state) {
>> +            case NEW:
>> +            case COMPLETING:
>> +                final Callable<?> callable = this.callable;
>> +                if (callable != null) {
>> +                    return start + "PENDING, task=[" + callable + "]]";
>> +                } else {
>> +                    return start + "PENDING]";
>> +                }
>> +            case NORMAL:
>> +                return start + "SUCCESS, result=[" + outcome + "]]";
>> +            case EXCEPTIONAL:
>> +                return start + "FAILURE, cause=[" + outcome + "]]";
>> +            case CANCELLED:
>> +            case INTERRUPTING:
>> +            case INTERRUPTED:
>> +                return start + "CANCELLED]";
>> +            default:
>> +                throw new IllegalStateException();
>> +            }
>> +        } catch (RuntimeException thrownFromToString) {
>> +            return start + "UNKNOWN, cause=["
>> +                + thrownFromToString.getClass() + "]";
>> +        }
>> +    }
>> +
>>      // VarHandle mechanics
>>      private static final VarHandle STATE;
>>      private static final VarHandle RUNNER;
>>      private static final VarHandle WAITERS;
>>      static {
>>          try {
>>              MethodHandles.Lookup l = MethodHandles.lookup();
>>              STATE = l.findVarHandle(FutureTask.class, "state",
>> int.class);
>>              RUNNER = l.findVarHandle(FutureTask.class, "runner",
>> Thread.class);
>>              WAITERS = l.findVarHandle(FutureTask.class, "waiters",
>> WaitNode.class);
>>
>>
>> _______________________________________________
>> Concurrency-interest mailing list
>> Concurrency-interest at cs.oswego.edu
>> http://cs.oswego.edu/mailman/listinfo/concurrency-interest
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://cs.oswego.edu/pipermail/concurrency-interest/attachments/20170809/ace25df2/attachment-0001.html>


More information about the Concurrency-interest mailing list