From cef7ba607ad7e8eb1ff2a438d77710a29aa0bda6 Mon Sep 17 00:00:00 2001 From: Sven Gothel Date: Mon, 22 Sep 2014 23:17:28 +0200 Subject: Fix synchronization issues in GLDrawableHelper.flushGLRunnables(), fixes rare deadlock with animator-exception and invoke(wait=true, ..) Fix synchronization issues in GLDrawableHelper.flushGLRunnables(): - Querying 'glRunnables.size()' is not synchronized, only its reference is volatile, not the instance's own states. - 'flushGLRunnable()' must operates while acquired the 'glRunnable' lock. - 'glRunnables' are no more volatile - introduced volatile 'glRunnableCount', allowing 'display(..)' method to pre-query whether blocking 'execGLRunnables(..)' must be called. This is risk (deadlock) free. Also fixes rare deadlock in animator display-exception / GLAD.invoke(wait=true, ..) case: - 'GLDrawableHelper.invoke(.., GLRunnable)' acquires the 'glRunnable' lock. - Then it queries animator state, which is blocking. - Hence animator's 'flushGLRunnable()' call must happen outside the animator lock --- .../classes/com/jogamp/opengl/util/Animator.java | 42 +++++++++++-------- .../com/jogamp/opengl/util/AnimatorBase.java | 9 ++-- .../com/jogamp/opengl/util/FPSAnimator.java | 49 ++++++++++++++-------- .../classes/jogamp/opengl/GLDrawableHelper.java | 44 ++++++++----------- 4 files changed, 78 insertions(+), 66 deletions(-) (limited to 'src') diff --git a/src/jogl/classes/com/jogamp/opengl/util/Animator.java b/src/jogl/classes/com/jogamp/opengl/util/Animator.java index c7a03eddb..4686e1745 100644 --- a/src/jogl/classes/com/jogamp/opengl/util/Animator.java +++ b/src/jogl/classes/com/jogamp/opengl/util/Animator.java @@ -165,7 +165,6 @@ public class Animator extends AnimatorBase { } catch (final UncaughtAnimatorException dre) { displayCaught = dre; stopIssued = true; - isAnimating = false; break; // end pause loop } } @@ -199,7 +198,6 @@ public class Animator extends AnimatorBase { } catch (final UncaughtAnimatorException dre) { displayCaught = dre; stopIssued = true; - isAnimating = false; break; // end animation loop } if ( !runAsFastAsPossible ) { @@ -226,24 +224,32 @@ public class Animator extends AnimatorBase { } } } - synchronized (Animator.this) { - if(DEBUG) { - System.err.println("Animator stop on " + animThread.getName() + ": " + toString()); - if( null != displayCaught ) { - System.err.println("Animator caught: "+displayCaught.getMessage()); - displayCaught.printStackTrace(); + boolean flushGLRunnables = false; + try { + synchronized (Animator.this) { + if(DEBUG) { + System.err.println("Animator stop on " + animThread.getName() + ": " + toString()); + if( null != displayCaught ) { + System.err.println("Animator caught: "+displayCaught.getMessage()); + displayCaught.printStackTrace(); + } } - } - stopIssued = false; - pauseIssued = false; - isAnimating = false; - try { - if( null != displayCaught ) { - handleUncaughtException(displayCaught); // may throw exception if null handler + stopIssued = false; + pauseIssued = false; + isAnimating = false; + try { + if( null != displayCaught ) { + flushGLRunnables = true; + handleUncaughtException(displayCaught); // may throw exception if null handler + } + } finally { + animThread = null; + Animator.this.notifyAll(); } - } finally { - animThread = null; - Animator.this.notifyAll(); + } + } finally { + if( flushGLRunnables ) { + flushGLRunnables(); } } } diff --git a/src/jogl/classes/com/jogamp/opengl/util/AnimatorBase.java b/src/jogl/classes/com/jogamp/opengl/util/AnimatorBase.java index 539d81beb..212a36ab4 100644 --- a/src/jogl/classes/com/jogamp/opengl/util/AnimatorBase.java +++ b/src/jogl/classes/com/jogamp/opengl/util/AnimatorBase.java @@ -524,23 +524,24 @@ public abstract class AnimatorBase implements GLAnimatorControl { /** * Should be called in case of an uncaught exception - * from within the animator thread to flush all animator + * from within the animator thread, throws given exception if no handler has been installed. */ protected final synchronized void handleUncaughtException(final UncaughtAnimatorException ue) { if( null != uncaughtExceptionHandler ) { try { uncaughtExceptionHandler.uncaughtException(this, ue.getGLAutoDrawable(), ue.getCause()); } catch (final Throwable t) { /* ignore intentionally */ } - flushGLRunnables(); } else { - flushGLRunnables(); throw ue; } } /** * Should be called in case of an uncaught exception - * from within the animator thread to flush all animator + * from within the animator thread to flush all animator. + *

+ * The animator instance shall not be locked when calling this method! + *

*/ protected final synchronized void flushGLRunnables() { for (int i=0; i off! } catch (final UncaughtAnimatorException dre) { - displayCaught = dre; + if(DEBUG) { + System.err.println("AnimatorBase.setExclusiveContextThread: caught: "+dre.getMessage()); + dre.printStackTrace(); + } + if( null == displayCaught ) { + displayCaught = dre; + } } } - synchronized (FPSAnimator.this) { - if(DEBUG) { - System.err.println("FPSAnimator stop " + Thread.currentThread() + ": " + toString()); - if( null != displayCaught ) { - System.err.println("AnimatorBase.setExclusiveContextThread: caught: "+displayCaught.getMessage()); - displayCaught.printStackTrace(); + boolean flushGLRunnables = false; + try { + synchronized (FPSAnimator.this) { + if(DEBUG) { + System.err.println("FPSAnimator stop " + Thread.currentThread() + ": " + toString()); + if( null != displayCaught ) { + System.err.println("Animator caught: "+displayCaught.getMessage()); + displayCaught.printStackTrace(); + } } - } - isAnimating = false; - try { - if( null != displayCaught ) { - handleUncaughtException(displayCaught); // may throw exception if null handler + isAnimating = false; + try { + if( null != displayCaught ) { + flushGLRunnables = true; + handleUncaughtException(displayCaught); // may throw exception if null handler + } + } finally { + animThread = null; + FPSAnimator.this.notifyAll(); } - } finally { - animThread = null; - FPSAnimator.this.notifyAll(); + } + } finally { + if( flushGLRunnables ) { + flushGLRunnables(); } } } diff --git a/src/jogl/classes/jogamp/opengl/GLDrawableHelper.java b/src/jogl/classes/jogamp/opengl/GLDrawableHelper.java index ec0d85c91..3deeafd27 100644 --- a/src/jogl/classes/jogamp/opengl/GLDrawableHelper.java +++ b/src/jogl/classes/jogamp/opengl/GLDrawableHelper.java @@ -78,7 +78,8 @@ public class GLDrawableHelper { private final ArrayList listeners = new ArrayList(); private final HashSet listenersToBeInit = new HashSet(); private final Object glRunnablesLock = new Object(); - private volatile ArrayList glRunnables = new ArrayList(); + private ArrayList glRunnables = new ArrayList(); + private volatile int glRunnableCount = 0; private boolean autoSwapBufferMode; private volatile Thread exclusiveContextThread; /** -1 release, 0 nop, 1 claim */ @@ -103,6 +104,7 @@ public class GLDrawableHelper { exclusiveContextThread = null; exclusiveContextSwitch = 0; synchronized(glRunnablesLock) { + glRunnableCount = 0; glRunnables.clear(); } animatorCtrl = null; @@ -669,7 +671,7 @@ public class GLDrawableHelper { public final void display(final GLAutoDrawable drawable) { displayImpl(drawable); // runForAllGLEventListener(drawable, displayAction); - if( glRunnables.size()>0 && !execGLRunnables(drawable) ) { // glRunnables volatile OK; execGL.. only executed if size > 0 + if( glRunnableCount > 0 && !execGLRunnables(drawable) ) { // glRunnableCount volatile OK; execGL.. only executed if size > 0 displayImpl(drawable); // runForAllGLEventListener(drawable, displayAction); } @@ -748,43 +750,29 @@ public class GLDrawableHelper { } private final boolean execGLRunnables(final GLAutoDrawable drawable) { // glRunnables.size()>0 - boolean res = true; // swap one-shot list asap final ArrayList _glRunnables; synchronized(glRunnablesLock) { - if(glRunnables.size()>0) { + if( glRunnables.size() > 0 ) { + glRunnableCount = 0; _glRunnables = glRunnables; glRunnables = new ArrayList(); } else { - _glRunnables = null; + return true; } } - - if(null!=_glRunnables) { - for (int i=0; i < _glRunnables.size(); i++) { - res = _glRunnables.get(i).run(drawable) && res; - } + boolean res = true; + for (int i=0; i < _glRunnables.size(); i++) { + res = _glRunnables.get(i).run(drawable) && res; } return res; } public final void flushGLRunnables() { - if(glRunnables.size()>0) { // volatile OK - // swap one-shot list asap - final ArrayList _glRunnables; - synchronized(glRunnablesLock) { - if(glRunnables.size()>0) { - _glRunnables = glRunnables; - glRunnables = new ArrayList(); - } else { - _glRunnables = null; - } - } - - if(null!=_glRunnables) { - for (int i=0; i < _glRunnables.size(); i++) { - _glRunnables.get(i).flush(); - } + synchronized(glRunnablesLock) { + glRunnableCount = 0; + while( glRunnables.size() > 0 ) { + glRunnables.remove(0).flush(); } } } @@ -914,6 +902,7 @@ public class GLDrawableHelper { rTask = new GLRunnableTask(glRunnable, wait ? rTaskLock : null, wait /* catch Exceptions if waiting for result */); + glRunnableCount++; glRunnables.add(rTask); } if( !deferredHere ) { @@ -978,11 +967,13 @@ public class GLDrawableHelper { wait = false; // don't wait if exec immediately } for(int i=0; i