From: Lloyd L Chambers <Lloyd.Chambers_at_Sun.COM>
Date: Tue, 11 Dec 2007 12:27:21 -0800

This fixes bug #3910: https://glassfish.dev.java.net/issues/
show_bug.cgi?id=3910

Diffs below. The key problem in question was _getOutput(), which had
a race condition which would cause it to not assign 'output' a value
if the first 'if' test found 'null' and the output then became non-
null (by another thread). Here is the revised method.

         private WrapOutput
     _getOutput( final String id )
     {
         WrapOutput output = mOutputs.get( id );
         /*
             A "fast path" is important here not for speed, but to
avoid the side-effects
             of taking a shared lock while debugging (which
serializes threads), thus changing
             the very behavior that might be being debugged. So we
want to take a lock
             only if the WrapOutput must be created.
          */
         if ( output == null )
         {
             // Did not exist at the time of the 'if' test above.
             // It might now exist; create a new WrapOutput
optimistically.
             output = new WrapOutput( getOutputFile( id ),
mDefaultDebug );

             // retain existing output if already present
             // Note that ConcurrentHashMap guarantees "happens
before" for put/get
             // so all fields of the WrapOutput will be visible to
other threads
             final WrapOutput prev = mOutputs.putIfAbsent( id, output );
             if ( prev != null )
             {
                 output = prev;
             }
             else
             {
                 debug( "AMXDebug: Created output for " +
StringUtil.quote( id ) );
             }
         }

         return output;
     }




MB2:/gf/build/glassfish/appserv-api lloyd$ cvs diff -uw src/java/com/
sun/appserv/management/base/AMXDebug.java
Index: src/java/com/sun/appserv/management/base/AMXDebug.java
===================================================================
RCS file: /cvs/glassfish/appserv-api/src/java/com/sun/appserv/
management/base/AMXDebug.java,v
retrieving revision 1.2
diff -u -w -r1.2 AMXDebug.java
--- src/java/com/sun/appserv/management/base/AMXDebug.java 5 May
2007 05:30:30 -0000 1.2
+++ src/java/com/sun/appserv/management/base/AMXDebug.java 11
Dec 2007 18:46:13 -0000
@@ -36,9 +36,10 @@
  package com.sun.appserv.management.base;

  import java.util.Map;
-import java.util.HashMap;
  import java.util.Set;
  import java.util.HashSet;
+import java.util.concurrent.ConcurrentMap;
+import java.util.concurrent.ConcurrentHashMap;

  import java.io.File;

@@ -134,7 +135,7 @@
   */
  public final class AMXDebug
  {
- private final Map<String,WrapOutput> mOutputs;
+ private final ConcurrentMap<String,WrapOutput> mOutputs;

      private static final AMXDebug INSTANCE = new AMXDebug();

@@ -216,7 +217,7 @@
             value = System.getProperty( AMX_DEBUG_APPEND_SPROP );
             mAppend = (value != null) && Boolean.parseBoolean
( value );

- mOutputs = new HashMap<String,WrapOutput>();
+ mOutputs = new ConcurrentHashMap<String,WrapOutput>();

          mDir = getDir();
          mMadeDebugDir = false;
@@ -506,23 +507,29 @@
      _getOutput( final String id )
      {
          WrapOutput output = mOutputs.get( id );
+ /*
+ A "fast path" is important here not for speed, but to
avoid the side-effects
+ of taking a shared lock while debugging (which
serializes threads), thus changing
+ the very behavior that might be being debugged. So we
want to take a lock
+ only if the WrapOutput must be created.
+ */
          if ( output == null )
          {
- synchronized( this )
- {
- if ( mOutputs.get( id ) == null )
- {
- debug( "Creating output for " + StringUtil.quote
( id ) );
- try
- {
+ // Did not exist at the time of the 'if' test above.
+ // It might now exist; create a new WrapOutput
optimistically.
                          output = new WrapOutput( getOutputFile
( id ), mDefaultDebug );
- mOutputs.put( id, output );
- }
- catch( Throwable t )
+
+ // retain existing output if already present
+ // Note that ConcurrentHashMap guarantees "happens
before" for put/get
+ // so all fields of the WrapOutput will be visible to
other threads
+ final WrapOutput prev = mOutputs.putIfAbsent( id, output );
+ if ( prev != null )
                      {
- debug( "Couldn't create output for " +
StringUtil.quote( id ) );
- }
+ output = prev;
                  }
+ else
+ {
+ debug( "AMXDebug: Created output for " +
StringUtil.quote( id ) );
              }
          }

@@ -579,15 +586,20 @@

      /**
          Internal class which wraps the Output so that
- debug may be dynamically enabled or disable without any
+ debug may be dynamically enabled or disabled without any
          users of the Output having to be aware of it.
       */
      public final class WrapOutput implements Output
      {
- private Output mWrapped;
- private File mFile;
+ private volatile Output mWrapped;
+
+ private final File mFile;
+
+ // protected by synchronized in checkStatus() and reset();
          private Output mFileOutput;
- private boolean mDebug;
+
+ // can be changed at any time
+ private volatile boolean mDebug;

                 private
         WrapOutput( final File file, final boolean debug )
@@ -646,7 +658,10 @@
             public synchronized void
         reset()
         {
+ // change output first!
             mWrapped = OutputIgnore.INSTANCE;
+
+ // OK, now mFileOutput is not used
             mFileOutput.close();

             // the new one is lazy-opened...
MB2:/gf/build/glassfish/appserv-api lloyd$

---
Lloyd L Chambers
lloyd.chambers_at_sun.com
Sun Microsystems, Inc