From aa4b8c9abbf9529fdbb3c4982895c01f2698451f Mon Sep 17 00:00:00 2001
From: Sven Gothel <sgothel@jausoft.com>
Date: Sun, 2 Dec 2012 04:09:41 +0100
Subject: NEWT EDTUtil: Simplify running state (default is running @
 setEDTUtil()); Simplify DefaultEDTUtil impl. and fix concurrency leak w/
 'shouldStop'

---
 src/newt/classes/com/jogamp/newt/Display.java      | 12 +--
 .../classes/com/jogamp/newt/swt/SWTEDTUtil.java    |  5 +-
 src/newt/classes/com/jogamp/newt/util/EDTUtil.java |  3 +-
 src/newt/classes/jogamp/newt/DefaultEDTUtil.java   | 99 +++++++++++-----------
 src/newt/classes/jogamp/newt/DisplayImpl.java      | 33 ++++----
 .../classes/jogamp/newt/driver/awt/AWTEDTUtil.java |  5 +-
 .../junit/newt/TestDisplayLifecycle01NEWT.java     |  4 +-
 7 files changed, 81 insertions(+), 80 deletions(-)

diff --git a/src/newt/classes/com/jogamp/newt/Display.java b/src/newt/classes/com/jogamp/newt/Display.java
index 4c263a195..993aa33eb 100644
--- a/src/newt/classes/com/jogamp/newt/Display.java
+++ b/src/newt/classes/com/jogamp/newt/Display.java
@@ -158,19 +158,15 @@ public abstract class Display {
      * </p>
      * <p>
      * If a previous one exists and it differs from the new one, 
-     * it's being stopped, wait-until-idle and reset to allow restart.
+     * it's being stopped, wait-until-idle and reset to allow a restart at a later time.
      * </p>
      * <p>
      * If <code>newEDTUtil</code> is not null and equals the previous one,
-     * <code>null</code> is returned and no change is being made.
+     * no change is being made.
      * </p>
      * <p>
-     * Note that <code>newEDTUtil</code> will not be started by this method.
-     * To do so you may issue {@link EDTUtil#invoke(boolean, Runnable) invoke} 
-     * on the new EDTUtil:
-     * <pre>
-     *          newEDTUtil.invoke(true, null);
-     * </pre>
+     * Note that <code>newEDTUtil</code> will be started by this method,
+     * if it is not running yet.
      * </p>
      */
     public abstract EDTUtil setEDTUtil(EDTUtil newEDTUtil);
diff --git a/src/newt/classes/com/jogamp/newt/swt/SWTEDTUtil.java b/src/newt/classes/com/jogamp/newt/swt/SWTEDTUtil.java
index 2203d744a..1c20fe524 100644
--- a/src/newt/classes/com/jogamp/newt/swt/SWTEDTUtil.java
+++ b/src/newt/classes/com/jogamp/newt/swt/SWTEDTUtil.java
@@ -150,7 +150,10 @@ public class SWTEDTUtil implements EDTUtil {
                 // System.err.println(Thread.currentThread()+" XXX stop: "+stop+", tasks: "+edt.tasks.size()+", task: "+task);
                 // Thread.dumpStack();
                 if(stop) {
-                    nedt.shouldStop = true;
+                    synchronized(nedt.sync) {
+                        nedt.shouldStop = true;
+                        nedt.sync.notifyAll(); // stop immediate if waiting (poll freq)
+                    }
                     if(DEBUG) {
                         System.err.println(Thread.currentThread()+": SWT-EDT signal STOP (on edt: "+isCurrentThreadEDT()+") - "+nedt);
                         // Thread.dumpStack();
diff --git a/src/newt/classes/com/jogamp/newt/util/EDTUtil.java b/src/newt/classes/com/jogamp/newt/util/EDTUtil.java
index 293e13592..0183da592 100644
--- a/src/newt/classes/com/jogamp/newt/util/EDTUtil.java
+++ b/src/newt/classes/com/jogamp/newt/util/EDTUtil.java
@@ -113,7 +113,8 @@ public interface EDTUtil {
 
     /** 
      * Append the final task to the EDT task queue,
-     * signals EDT to stop and wait until stopped.<br>
+     * signals EDT to stop and wait until stopped.<br/>
+     * <code>task</code> maybe <code>null</code><br/>
      * Due to the nature of this method:
      * <ul>
      *   <li>All previous queued tasks will be finished.</li>
diff --git a/src/newt/classes/jogamp/newt/DefaultEDTUtil.java b/src/newt/classes/jogamp/newt/DefaultEDTUtil.java
index b141f9d29..d8d04e79f 100644
--- a/src/newt/classes/jogamp/newt/DefaultEDTUtil.java
+++ b/src/newt/classes/jogamp/newt/DefaultEDTUtil.java
@@ -135,6 +135,11 @@ public class DefaultEDTUtil implements EDTUtil {
         invokeImpl(wait, task, false);
     }
 
+    private static Runnable nullTask = new Runnable() {
+        @Override
+        public void run() { }        
+    };
+    
     private void invokeImpl(boolean wait, Runnable task, boolean stop) {
         Throwable throwable = null;
         RunnableTask rTask = null;
@@ -151,39 +156,50 @@ public class DefaultEDTUtil implements EDTUtil {
                 }
                 // System.err.println(Thread.currentThread()+" XXX stop: "+stop+", tasks: "+edt.tasks.size()+", task: "+task);
                 // Thread.dumpStack();
-                if(stop) {
-                    edt.shouldStop = true;
-                    if(DEBUG) {
-                        System.err.println(Thread.currentThread()+": Default-EDT signal STOP (on edt: "+isCurrentThreadEDT()+") - tasks: "+edt.tasks.size()+" - "+edt);
-                        // Thread.dumpStack();
+                if( isCurrentThreadEDT() ) {
+                    if(null != task) {
+                        task.run();
                     }
-                } else if( !edt.isRunning() ) {
-                    // start if should not stop && not started yet
-                    startImpl();
-                }
-                if( null == task ) {
-                    wait = false;
-                } else if( isCurrentThreadEDT() ) {
-                    task.run();
                     wait = false; // running in same thread (EDT) -> no wait
-                    if(stop && edt.tasks.size()>0) {
-                        System.err.println(Thread.currentThread()+": Warning: Default-EDT about (2) to stop, having remaining tasks: "+edt.tasks.size()+" - "+edt);
-                        if(DEBUG) {
-                            Thread.dumpStack();
+                    if(stop) {
+                        edt.shouldStop = true;
+                        if( edt.tasks.size()>0 ) {
+                            System.err.println(Thread.currentThread()+": Warning: Default-EDT about (2) to stop, task executed. Remaining tasks: "+edt.tasks.size()+" - "+edt);
+                            if(DEBUG) {
+                                Thread.dumpStack();
+                            }
                         }
                     }
                 } else {
-                    synchronized(edt.tasks) {
-                        wait = wait && edt.isRunning();
-                        rTask = new RunnableTask(task,
-                                                 wait ? rTaskLock : null,
-                                                 true /* always catch and report Exceptions, don't disturb EDT */);
-                        if(stop) {
-                            rTask.setAttachment(new Boolean(true)); // mark final task
+                    if( !edt.isRunning() ) {
+                        if( !stop ) {
+                            startImpl();
+                        } else {
+                            // drop task and don't wait
+                            task = null;
+                            System.err.println(Thread.currentThread()+": Warning: Default-EDT is about (3) to stop and stopped already, dropping task. Remaining tasks: "+edt.tasks.size()+" - "+edt);
+                            if(true || DEBUG) {
+                                Thread.dumpStack();
+                            }
+                        }
+                    } else if(stop && null == task) {
+                        task = nullTask;
+                    }
+
+                    if(null != task) {
+                        synchronized(edt.tasks) {
+                            rTask = new RunnableTask(task,
+                                                     wait ? rTaskLock : null,
+                                                     true /* always catch and report Exceptions, don't disturb EDT */);
+                            if(stop) {
+                                rTask.setAttachment(new Boolean(true)); // mark final task, will imply shouldStop:=true
+                            }
+                            // append task ..
+                            edt.tasks.add(rTask);
+                            edt.tasks.notifyAll();
                         }
-                        // append task ..
-                        edt.tasks.add(rTask);
-                        edt.tasks.notifyAll();
+                    } else {
+                        wait = false;
                     }
                 }
             }
@@ -305,6 +321,9 @@ public class DefaultEDTUtil implements EDTUtil {
                         if(tasks.size()>0) {
                             task = tasks.remove(0);
                             tasks.notifyAll();
+                            if( null != task.getAttachment() ) {
+                                shouldStop = true;
+                            }
                         }
                     }
                     if(null!=task) {
@@ -330,30 +349,8 @@ public class DefaultEDTUtil implements EDTUtil {
                     System.err.println(getName()+": Default-EDT run() END "+ getName()+", tasks: "+tasks.size()+", "+rt+", "+error); 
                 }
                 synchronized(edtLock) {
-                    if(null==error) {
-                        synchronized(tasks) {
-                            // drain remaining tasks (stop not on EDT), 
-                            // while having tasks and no previous-task, or previous-task is non final
-                            RunnableTask task = null;
-                            while ( ( null == task || task.getAttachment() == null ) && tasks.size() > 0 ) {
-                                task = tasks.remove(0);
-                                task.run();
-                                tasks.notifyAll();
-                            }
-                            if(DEBUG) {
-                                if(null!=task && task.getAttachment()==null) {
-                                    System.err.println(getName()+" Warning: Default-EDT exit: Last task Not Final: "+tasks.size()+", "+task+" - "+edt);
-                                } else if(tasks.size()>0) {
-                                    System.err.println(getName()+" Warning: Default-EDT exit: Remaining tasks Post Final: "+tasks.size());
-                                }
-                                Thread.dumpStack();
-                            }
-                        }
-                    }
-                    isRunning = !shouldStop;
-                    if(!isRunning) {
-                        edtLock.notifyAll();
-                    }
+                    isRunning = false;
+                    edtLock.notifyAll();
                 }
                 if(DEBUG) {
                     System.err.println(getName()+": Default-EDT run() EXIT "+ getName()+", exception: "+error);
diff --git a/src/newt/classes/jogamp/newt/DisplayImpl.java b/src/newt/classes/jogamp/newt/DisplayImpl.java
index 317535805..20b915cae 100644
--- a/src/newt/classes/jogamp/newt/DisplayImpl.java
+++ b/src/newt/classes/jogamp/newt/DisplayImpl.java
@@ -84,9 +84,8 @@ public abstract class DisplayImpl extends Display {
                 display.hashCode = display.fqname.hashCode();
                 displayList.add(display);
             }
-            if(null == display.edtUtil) {
-                display.setEDTUtil(null); // device's default if EDT is used, or null        
-            }
+            display.setEDTUtil(display.edtUtil); // device's default if EDT is used, or null
+            
             if(DEBUG) {
                 System.err.println("Display.create() NEW: "+display+" "+getThreadName());
             }
@@ -166,20 +165,24 @@ public abstract class DisplayImpl extends Display {
 
     @Override
     public EDTUtil setEDTUtil(EDTUtil newEDTUtil) {        
+        final EDTUtil oldEDTUtil = edtUtil;
         if(null == newEDTUtil) {
-            newEDTUtil = createEDTUtil();
-        } else if( newEDTUtil == edtUtil ) {
             if(DEBUG) {
-                System.err.println("Display.setEDTUtil: "+newEDTUtil+" - keep!");
+                System.err.println("Display.setEDTUtil(default): "+oldEDTUtil+" -> "+newEDTUtil);
+            }
+            edtUtil = createEDTUtil();
+        } else if( newEDTUtil != edtUtil ) {
+            if(DEBUG) {
+                System.err.println("Display.setEDTUtil(custom): "+oldEDTUtil+" -> "+newEDTUtil);
             }
-            return null; // no change
+            removeEDT( null );
+            edtUtil = newEDTUtil;
+        } else if( DEBUG ) {
+            System.err.println("Display.setEDTUtil: "+newEDTUtil+" - keep!");
         }
-        final EDTUtil oldEDTUtil = edtUtil;
-        if(DEBUG) {
-            System.err.println("Display.setEDTUtil: "+oldEDTUtil+" -> "+newEDTUtil);
+        if( !edtUtil.isRunning() ) { // start EDT if not running yet
+            edtUtil.invoke(true, null);
         }
-        removeEDT( new Runnable() { public void run() {} } );
-        edtUtil = newEDTUtil;
         return oldEDTUtil;
     }
 
@@ -209,11 +212,7 @@ public abstract class DisplayImpl extends Display {
 
     public boolean validateEDT() {
         if(0==refCount && null==aDevice && null != edtUtil && edtUtil.isRunning()) {
-            removeEDT( new Runnable() {
-                public void run() {
-                    // nop
-                }
-            } );
+            removeEDT( null );
             return true;
         }
         return false;
diff --git a/src/newt/classes/jogamp/newt/driver/awt/AWTEDTUtil.java b/src/newt/classes/jogamp/newt/driver/awt/AWTEDTUtil.java
index cb9bc90b8..27d0f3506 100644
--- a/src/newt/classes/jogamp/newt/driver/awt/AWTEDTUtil.java
+++ b/src/newt/classes/jogamp/newt/driver/awt/AWTEDTUtil.java
@@ -140,7 +140,10 @@ public class AWTEDTUtil implements EDTUtil {
                 // System.err.println(Thread.currentThread()+" XXX stop: "+stop+", tasks: "+edt.tasks.size()+", task: "+task);
                 // Thread.dumpStack();
                 if(stop) {
-                    nedt.shouldStop = true;
+                    synchronized(nedt.sync) {
+                        nedt.shouldStop = true;
+                        nedt.sync.notifyAll(); // stop immediate if waiting (poll freq)
+                    }
                     if(DEBUG) {
                         System.err.println(Thread.currentThread()+": AWT-EDT signal STOP (on edt: "+isCurrentThreadEDT()+") - "+nedt);
                         // Thread.dumpStack();
diff --git a/src/test/com/jogamp/opengl/test/junit/newt/TestDisplayLifecycle01NEWT.java b/src/test/com/jogamp/opengl/test/junit/newt/TestDisplayLifecycle01NEWT.java
index c9450c2d6..e4867e3fe 100644
--- a/src/test/com/jogamp/opengl/test/junit/newt/TestDisplayLifecycle01NEWT.java
+++ b/src/test/com/jogamp/opengl/test/junit/newt/TestDisplayLifecycle01NEWT.java
@@ -89,7 +89,7 @@ public class TestDisplayLifecycle01NEWT extends UITestCase {
         Assert.assertEquals(0,display.getReferenceCount());
         Assert.assertEquals(false,display.isNativeValid());
         Assert.assertNotNull(display.getEDTUtil());
-        Assert.assertEquals(false,display.getEDTUtil().isRunning());
+        Assert.assertEquals(true,display.getEDTUtil().isRunning());
         Assert.assertEquals(0,screen.getReferenceCount());
         Assert.assertEquals(false,screen.isNativeValid());
 
@@ -201,6 +201,8 @@ public class TestDisplayLifecycle01NEWT extends UITestCase {
         Assert.assertEquals(false,display.isNativeValid());
         Assert.assertNotNull(display.getEDTUtil());
         Assert.assertEquals(false,display.getEDTUtil().isRunning());
+        display.getEDTUtil().invoke(true, null);
+        Assert.assertEquals(true,display.getEDTUtil().isRunning());
         Assert.assertEquals(0,screen.getReferenceCount());
         Assert.assertEquals(false,screen.isNativeValid());
 
-- 
cgit v1.2.3