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

Martin Buchholz martinrb at google.com
Fri Aug 11 19:16:19 EDT 2017


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/304c634d977127085b49f174fd1aaf
>>>>> efd09b2bf5
>>>>>
>>>>> 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/jsr166/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/jsr166/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/jsr166/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/7ff77747/attachment-0001.html>


More information about the Concurrency-interest mailing list