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

Benjamin Manes ben.manes at gmail.com
Wed Aug 9 13:29:54 EDT 2017


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/20170809/11f999a3/attachment-0001.html>


More information about the Concurrency-interest mailing list