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

Joe Bowbeer joe.bowbeer at gmail.com
Fri Aug 11 13:09:48 EDT 2017


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/304c634d977127085b49f174fd1aafefd09b2bf5
>>>>
>>>> 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
>>>>>> PrivilegedCallableUsingCurrentClassLoader<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/aef284c9/attachment-0001.html>


More information about the Concurrency-interest mailing list