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

Joe Bowbeer joe.bowbeer at gmail.com
Fri Aug 11 19:31:14 EDT 2017


I would not assume that calling toString is safe or desirable, for the
various reasons I listed.

These toString implementations have not existed for a dozen years, yet
their lack of existence has never been mentioned on this list (right?), so
I would err on the side of caution.

On Aug 11, 2017 4:16 PM, "Martin Buchholz" <martinrb at google.com> wrote:

> I propose the variant below, which is generally useful and assumes that
> calling toString on tasks and exceptions is safe, but calling toString on
> results is not.  If y'all are happy with that, we can add docs (for
> FutureTask.toString) and tests.
>
> Index: Executors.java
> ===================================================================
> RCS file: /export/home/jsr166/jsr166/jsr166/src/main/java/util/
> concurrent/Executors.java,v
> retrieving revision 1.97
> diff -u -r1.97 Executors.java
> --- Executors.java 15 Apr 2017 00:12:38 -0000 1.97
> +++ Executors.java 11 Aug 2017 23:12:14 -0000
> @@ -485,6 +485,9 @@
>              task.run();
>              return result;
>          }
> +        public String toString() {
> +            return super.toString() + "[wrapped task = " + task + "]";
> +        }
>      }
>
>      /**
> @@ -511,6 +514,10 @@
>                  throw e.getException();
>              }
>          }
> +
> +        public String toString() {
> +            return super.toString() + "[wrapped task = " + task + "]";
> +        }
>      }
>
>      /**
> @@ -563,6 +570,10 @@
>                  throw e.getException();
>              }
>          }
> +
> +        public String toString() {
> +            return super.toString() + "[wrapped task = " + task + "]";
> +        }
>      }
>
>      /**
> Index: ForkJoinTask.java
> ===================================================================
> RCS file: /export/home/jsr166/jsr166/jsr166/src/main/java/util/
> concurrent/ForkJoinTask.java,v
> retrieving revision 1.115
> diff -u -r1.115 ForkJoinTask.java
> --- ForkJoinTask.java 19 Apr 2017 23:45:51 -0000 1.115
> +++ ForkJoinTask.java 11 Aug 2017 23:12:14 -0000
> @@ -1346,6 +1346,9 @@
>          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() + "[wrapped task = " + runnable + "]";
> +        }
>          private static final long serialVersionUID = 5232453952276885070L;
>      }
>
> @@ -1363,6 +1366,9 @@
>          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() + "[wrapped task = " + runnable + "]";
> +        }
>          private static final long serialVersionUID = 5232453952276885070L;
>      }
>
> @@ -1409,6 +1415,9 @@
>          }
>          public final void run() { invoke(); }
>          private static final long serialVersionUID = 2838392045355241008L;
> +        public String toString() {
> +            return super.toString() + "[wrapped task = " + callable + "]";
> +        }
>      }
>
>      /**
> Index: FutureTask.java
> ===================================================================
> RCS file: /export/home/jsr166/jsr166/jsr166/src/main/java/util/
> concurrent/FutureTask.java,v
> retrieving revision 1.118
> diff -u -r1.118 FutureTask.java
> --- FutureTask.java 10 Sep 2016 04:06:51 -0000 1.118
> +++ FutureTask.java 11 Aug 2017 23:12:14 -0000
> @@ -451,6 +451,27 @@
>          }
>      }
>
> +    public String toString() {
> +        final String status;
> +        switch (state) {
> +        case NORMAL:
> +            status = "[completed normally]";
> +            break;
> +        case EXCEPTIONAL:
> +            status = "[completed exceptionally: " + outcome + "]";
> +            break;
> +        case CANCELLED:
> +            status = "[cancelled]";
> +            break;
> +        default:
> +            final Callable<?> callable = this.callable;
> +            status = (callable == null)
> +                ? "[incomplete]"
> +                : "[incomplete, task = " + callable + "]";
> +        }
> +        return super.toString() + status;
> +    }
> +
>      // VarHandle mechanics
>      private static final VarHandle STATE;
>      private static final VarHandle RUNNER;
>
>
> On Fri, Aug 11, 2017 at 10:09 AM, Joe Bowbeer <joe.bowbeer at gmail.com>
> wrote:
>
>> Yes, that minimal enhancement seems safe and efficient.
>>
>> On Wed, Aug 9, 2017 at 10:55 AM Charles Munger <clm at google.com> wrote:
>>
>>> How about starting by adding the following toString method to the
>>> adapter classes:
>>>
>>> @Override
>>> public String toString() {
>>>    return super.toString() + "[" + task.getClass().getName() + '@' +
>>> Integer.toHexString(System.identityHashCode(task)) + "]";
>>> }
>>>
>>> That produces bounded output size, exposes no PII, doesn't risk an
>>> exception or thread safety issues, and still provides enhanced debugging
>>> output. In fact, for nearly all implementations of Runnable and Callable,
>>> the class name and identity hash is all the useful output that someone
>>> needs.
>>>
>>> On Wed, Aug 9, 2017 at 10:29 AM, Benjamin Manes <ben.manes at gmail.com>
>>> wrote:
>>>
>>>> I would be comfortable with a toString() showing the task's class and
>>>> other high level state of owned fields, but not if calling foreign code
>>>> like the value's toString(). However, I can't think of a case where this
>>>> would have significantly helpful when debugging, so I am neutral between
>>>> minimal or no changes.
>>>>
>>>> On Wed, Aug 9, 2017 at 10:10 AM, Joe Bowbeer <joe.bowbeer at gmail.com>
>>>> wrote:
>>>>
>>>>> RunnableAdapter is an interesting case. I'd have most of the same
>>>>> concerns as before, to a slightly less extent. This would not be an API
>>>>> change, however.
>>>>>
>>>>> The style adopted by Java so far has been that j.u.c. objects don't by
>>>>> default reveal their internal state. It has been up to devs to opt in,
>>>>> based on their particular needs, and some devs rightly or wrongly rely on
>>>>> the format inherited from Object.
>>>>>
>>>>> If Guava or other libraries want to enhance this, that can be their
>>>>> value added.
>>>>>
>>>>> By the way, adding to my previous list of concerns, I'd also be
>>>>> concerned about leaking PI in log messages. This is related to my concern
>>>>> about the wrapped toString impl. being designed for use in a different
>>>>> context.
>>>>>
>>>>> On Wed, Aug 9, 2017 at 9:19 AM Charles Munger <clm at google.com> wrote:
>>>>>
>>>>>> I added similar toString implementations to guava's Future
>>>>>> implementations:
>>>>>> https://github.com/google/guava/commit/304c634d977127085b49f
>>>>>> 174fd1aafefd09b2bf5
>>>>>>
>>>>>> The attraction of toString on common Future implementations is that
>>>>>> propagating toString throughout a graph of transformed Futures lets you see
>>>>>> what a task is doing before it completes, either in a TimeoutException
>>>>>> message or the toString of LockSupport.getBlocker.
>>>>>>
>>>>>> Do you feel that adding toString to RunnableAdapter (and similar
>>>>>> classes, that are only exposed in public API as interfaces) would also
>>>>>> constitute an API change?
>>>>>>
>>>>>> 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/jsr
>>>>>>>> 166/src/main/java/util/concurrent/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/jsr
>>>>>>>> 166/src/main/java/util/concurrent/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/jsr
>>>>>>>> 166/src/main/java/util/concurrent/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
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>> _______________________________________________
>>>>> 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/20170811/14cdc038/attachment-0001.html>


More information about the Concurrency-interest mailing list