DO NOT REPLY [Bug 23092] New: - Map.debug/verbosePrint Thread Safety

[bu...@apache.org]

DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG 
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
<http://nagoya.apache.org/bugzilla/show_bug.cgi?id=23092>.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND 
INSERTED IN THE BUG DATABASE.

http://nagoya.apache.org/bugzilla/show_bug.cgi?id=23092

Map.debug/verbosePrint Thread Safety

           Summary: Map.debug/verbosePrint Thread Safety
           Product: Commons
           Version: unspecified
          Platform: Other
        OS/Version: Other
            Status: NEW
          Severity: Normal
          Priority: Other
         Component: Collections
        AssignedTo: commons-dev@jakarta.apache.org
        ReportedBy: apacheBugzilla@AMammenT.cotse.net


This bug records a discussion on the mailing list, and provides a forum for 
continuing that discussion.  I include below the content of the mailing on the 
list because they raise some interesting points.  

-----------------------------------------------------
From: "Arun Thomas" <Ar...@solidusnetworks.com>
-----------------------------------------------------

I recently noticed that the current implementation of the debugPrint and 
verbosePrint methods in MapUtils is not thread safe - in fact, these are the 
only items that are not thread safe because the indentation state for the 
output is preserved in a static variable. 

I think it would be fairly simple to make this thread safe, but it would 
require a change to the signature of a protected member function printIndent
(PrintStream out) -> printIndent(PrintStream out, int indentDepth).  I'm not 
really clear as to why this method is protected rather than private (can 
anyone clarify?).  Does it need to be protected? 

Looking forward to input from others.
-AMT 

------------------------------------------------------
From: Stephen Colebourne [scolebourne@btopenworld.com]
------------------------------------------------------

The methods that call this one are synchronized to protect them, so it
should be thread-safe.

However, I would prefer them not to need to be synchronized. If you want to
submit a patch that reworks the methods that would be great.

Incompatable change?
The protected method should remain and be deprecated, as it may be used by a
MapUtils subclass (feasible but unlikely). However, I would prefer just to
go incompatable change on this one. Other committers may differ, but if they
don't I'll just commit the incompatable change.

Stephen

---------------------------------------
From: Janek Bogucki [yan@studylink.com]
---------------------------------------

These methods are synchronized so are currently thread safe.

    public static synchronized void verbosePrint (...)

    public static synchronized void debugPrint (...)

I would guess that a second intentional benefit of the synchronization
is to prevent overlapping invocations writing to System.out at the same
time.

-Janek

------------------------------------------------------
From: Stephen Colebourne [scolebourne@btopenworld.com]
------------------------------------------------------

I like the System.out theory. Its probably unintentional, but it true. So
only a documentation patch is required then...:-)
Stephen