Adding toString methods for task objects?

classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|

Adding toString methods for task objects?

Martin Buchholz-3
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
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Adding toString methods for task objects?

Joe Bowbeer
​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 <[hidden email]> 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
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest



_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Adding toString methods for task objects?

Joe Bowbeer
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 <[hidden email]> wrote:
I added similar toString implementations to guava's Future implementations:

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 <[hidden email]> 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 <[hidden email]> 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
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest




_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Adding toString methods for task objects?

Benjamin Manes
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 <[hidden email]> 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 <[hidden email]> wrote:
I added similar toString implementations to guava's Future implementations:

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 <[hidden email]> 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 <[hidden email]> 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
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest




_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest



_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Adding toString methods for task objects?

Martin Buchholz-3
In reply to this post by Joe Bowbeer
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 <[hidden email]> 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 <[hidden email]> 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
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest




_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Adding toString methods for task objects?

Joe Bowbeer
In reply to this post by Benjamin Manes
Yes, that minimal enhancement seems safe and efficient.

On Wed, Aug 9, 2017 at 10:55 AM Charles Munger <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> wrote:
I added similar toString implementations to guava's Future implementations:

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 <[hidden email]> 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 <[hidden email]> 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
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest




_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest




_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Adding toString methods for task objects?

Martin Buchholz-3
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 <[hidden email]> wrote:
Yes, that minimal enhancement seems safe and efficient.

On Wed, Aug 9, 2017 at 10:55 AM Charles Munger <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> wrote:
I added similar toString implementations to guava's Future implementations:

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 <[hidden email]> 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 <[hidden email]> 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
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest




_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest





_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Adding toString methods for task objects?

Joe Bowbeer
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" <[hidden email]> 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 <[hidden email]> wrote:
Yes, that minimal enhancement seems safe and efficient.

On Wed, Aug 9, 2017 at 10:55 AM Charles Munger <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> wrote:
I added similar toString implementations to guava's Future implementations:

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 <[hidden email]> 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 <[hidden email]> 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
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest




_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest





_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Adding toString methods for task objects?

Joe Bowbeer
Martin, it wasn't clear to me whether you are proposing this for some future major release or not. Java 10, for example, or whatever the next train is. That seems like the right time to make the broader change that you are suggesting, which, as you say wrote, has the potential for trouble.


On Fri, Aug 11, 2017 at 4:31 PM, Joe Bowbeer <[hidden email]> wrote:
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" <[hidden email]> 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 <[hidden email]> wrote:
Yes, that minimal enhancement seems safe and efficient.

On Wed, Aug 9, 2017 at 10:55 AM Charles Munger <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> wrote:
I added similar toString implementations to guava's Future implementations:

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 <[hidden email]> 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 <[hidden email]> 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
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest




_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest






_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Adding toString methods for task objects?

Martin Buchholz-3
Joe, yes, this is a jdk10 proposal.

An argument for printing exceptions is that those are especially useful for debugging and inversely, are particularly likely to be designed to have useful string representations.

An argument for propagating toString contents in adapter classes is that the adapter class should be as much as possible a drop-in replacement for the wrapped class, and the wrapper class will not have a toString method that is less safe than the wrapped class.

Latest version:

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 12 Aug 2017 01:08:34 -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 12 Aug 2017 01:08:34 -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 12 Aug 2017 01:08:34 -0000
@@ -451,6 +451,29 @@
         }
     }
 
+    public String toString() {
+        final String status;
+        switch (state) {
+        case NORMAL:
+            status = "[completed normally]";
+            break;
+        case EXCEPTIONAL:
+            status = "[completed exceptionally: " + outcome + "]";
+            break;
+        case CANCELLED:
+        case INTERRUPTING:
+        case INTERRUPTED:
+            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 5:08 PM, Joe Bowbeer <[hidden email]> wrote:
Martin, it wasn't clear to me whether you are proposing this for some future major release or not. Java 10, for example, or whatever the next train is. That seems like the right time to make the broader change that you are suggesting, which, as you say wrote, has the potential for trouble.


On Fri, Aug 11, 2017 at 4:31 PM, Joe Bowbeer <[hidden email]> wrote:
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" <[hidden email]> 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 <[hidden email]> wrote:
Yes, that minimal enhancement seems safe and efficient.

On Wed, Aug 9, 2017 at 10:55 AM Charles Munger <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> wrote:
I added similar toString implementations to guava's Future implementations:

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 <[hidden email]> 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 <[hidden email]> 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
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest




_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest







_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Adding toString methods for task objects?

Carfield Yim
In reply to this post by Joe Bowbeer
Sorry, just a side question, if toString() is not used for debug, what is the original propose of having it?

On Wed, Aug 9, 2017 at 9:58 AM, Joe Bowbeer <[hidden email]> 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 <[hidden email]> 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
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest



_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest



_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Adding toString methods for task objects?

Joe Bowbeer
toString is an expedient string representation that in practice can be used for a lot of things, including formatted output, log statements, debug output, stack traces, and so on, as the need arises.

On Aug 11, 2017 10:01 PM, "Carfield Yim" <[hidden email]> wrote:
Sorry, just a side question, if toString() is not used for debug, what is the original propose of having it?

On Wed, Aug 9, 2017 at 9:58 AM, Joe Bowbeer <[hidden email]> 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 <[hidden email]> 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
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest



_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest




_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Adding toString methods for task objects?

Martin Buchholz-3
In reply to this post by Martin Buchholz-3
We now have a serious proposal, with docs and tests.


CompletableFuture.toString already included status, so aligned FutureTask.toString with that.

_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Adding toString methods for task objects?

Doug Lea
On 08/16/2017 02:30 PM, Martin Buchholz wrote:
> We now have a serious proposal, with docs and tests.
>
> http://cr.openjdk.java.net/~martin/webrevs/openjdk10/jsr166-integration/toString-moredata/
>

I hadn't replied initially, wanting to hear of any unknown reason
it might be problematic, but given responses, I think this is worth
doing.

-Doug


> CompletableFuture.toString already included status, so aligned
> FutureTask.toString with that.
>
>
> _______________________________________________
> Concurrency-interest mailing list
> [hidden email]
> http://cs.oswego.edu/mailman/listinfo/concurrency-interest
>

_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest