Approved. Excellent attention to niggling details.
Lloyd L Chambers wrote:
> 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 <mailto:lloyd.chambers_at_sun.com>
> Sun Microsystems, Inc
>
>
>
--
Byron Nevins Work 408-276-4089, Home 650-359-1290, Cell 650-784-4123 - Sun Microsystems, Inc.