Mailing-List: contact issues-help@commons.apache.org; run by ezmlm
Precedence: bulk
Reply-To: issues@commons.apache.org
Date: Thu, 10 Nov 2011 03:29:51 +0000 (UTC)
From: "Phil Steitz (Commented) (JIRA)" <jira@apache.org>
To: issues@commons.apache.org
Message-ID: 
 <61935384.16435.1320895791736.JavaMail.tomcat@hel.zones.apache.org>
In-Reply-To: 
 <270085597.1389.1318875671085.JavaMail.tomcat@hel.zones.apache.org>
Subject: [jira] [Commented] (LANG-762) Handle or document
 ReflectionToStringBuilder and ToStringBuilder for collections that are not
 thread safe
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit


    [ https://issues.apache.org/jira/browse/LANG-762?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13147468#comment-13147468 ] 

Phil Steitz commented on LANG-762:
----------------------------------

Looking at this again, I am pretty well convinced that [lang] is never going to be able to "fix" this issue.  Consider the case where a class uses its own private Lock instances to protect data members.  Unless you want to get into the byte code analysis business, you are not going to be able to pick this up or access the relevant locks.  Moreoever, even if you could discern and acquire the right locks, you might risk introducing deadlocks or liveness problems for the code, because the class may have lock acquisition / release order invariants that you don't know about.

The risks associated with using the ReflectionToStringBuilder to access fields protected by synchronization should be documented. I would suggest just adding something like the following to the class javadoc, after the sentence that reads "This will fail under a security manager, unless the appropriate permissions are set up correctly" add "Using reflection to access private fields also circumvents any synchronization protection guarding access to private fields.  Fields that cannot safely be read at any time by toString should be excluded from the generated method, or synchronization consistent with the underlying class' lock management should be added around invocation of the method. Special care should be taken to avoid including non-threadsafe collection classes, as these classes may throw ConcurrentModificationException if modified while the toString method is executing."

Above is probably too long, but something like it should be added.
                
> Handle or document ReflectionToStringBuilder and ToStringBuilder for collections that are not thread safe
> ---------------------------------------------------------------------------------------------------------
>
>                 Key: LANG-762
>                 URL: https://issues.apache.org/jira/browse/LANG-762
>             Project: Commons Lang
>          Issue Type: Bug
>          Components: lang.builder.*
>         Environment: Apache Maven 3.0.3 (r1075438; 2011-02-28 12:31:09-0500)
> Maven home: C:\Java\apache-maven-3.0.3\bin\..
> Java version: 1.6.0_24, vendor: Sun Microsystems Inc.
> Java home: C:\Program Files\Java\jdk1.6.0_24\jre
> Default locale: en_US, platform encoding: Cp1252
> OS name: "windows 7", version: "6.1", arch: "amd64", family: "windows"
>            Reporter: Gary D. Gregory
>         Attachments: ReflectionToStringBuilderConcurrencyTest.java, patch-LANG762.txt
>
>
> Moving discussion here from https://issues.apache.org/jira/browse/POOL-191 ConcurrentModificationException in GenericObjectPool LinkedList.
> It is possible to get a {{ConcurrentModificationException}} in a {{LinkedList}} from a Commons Pool {{GenericObjectPool}}.
> This happens when I call {{ReflectionToStringBuilder.toString(this)}} from a subclass of {{GenericObjectPool}}. My guess is that it would happen with just {{ReflectionToStringBuilder.toString(gop)}}. IOW, subclassing does not have anything to do with it I would venture.
> For example, in this stack trace {{JmsSessionPool}} is a subclass of {{GenericObjectPool}}.
> {noformat}
> java.util.ConcurrentModificationException
> at java.util.LinkedList$ListItr.checkForComodification(LinkedList.java:761)
> at java.util.LinkedList$ListItr.next(LinkedList.java:696)
> at java.util.AbstractCollection.toString(AbstractCollection.java:421)
> at java.lang.String.valueOf(String.java:2826)
> at java.lang.StringBuffer.append(StringBuffer.java:219)
> at org.apache.commons.lang3.builder.ToStringStyle.appendDetail(ToStringStyle.java:598)
> at org.apache.commons.lang3.builder.ToStringStyle.appendInternal(ToStringStyle.java:473)
> at org.apache.commons.lang3.builder.ToStringStyle.append(ToStringStyle.java:436)
> at org.apache.commons.lang3.builder.ToStringBuilder.append(ToStringBuilder.java:848)
> at org.apache.commons.lang3.builder.ReflectionToStringBuilder.appendFieldsIn(ReflectionToStringBuilder.java:528)
> at org.apache.commons.lang3.builder.ReflectionToStringBuilder.toString(ReflectionToStringBuilder.java:692)
> at org.apache.commons.lang3.builder.ReflectionToStringBuilder.toString(ReflectionToStringBuilder.java:288)
> at org.apache.commons.lang3.builder.ReflectionToStringBuilder.toString(ReflectionToStringBuilder.java:119)
> at com.seagullsw.appinterface.comm.jms.JmsSessionPool.toString(JmsSessionPool.java:120)
> at java.lang.String.valueOf(String.java:2826)
> at java.lang.StringBuffer.append(StringBuffer.java:219)
> at org.apache.commons.lang3.builder.ToStringStyle.appendDetail(ToStringStyle.java:586)
> at org.apache.commons.lang3.builder.ToStringStyle.appendInternal(ToStringStyle.java:550)
> at org.apache.commons.lang3.builder.ToStringStyle.append(ToStringStyle.java:436)
> at org.apache.commons.lang3.builder.ToStringBuilder.append(ToStringBuilder.java:848)
> at org.apache.commons.lang3.builder.ReflectionToStringBuilder.appendFieldsIn(ReflectionToStringBuilder.java:528)
> at org.apache.commons.lang3.builder.ReflectionToStringBuilder.toString(ReflectionToStringBuilder.java:689)
> at org.apache.commons.lang3.builder.ReflectionToStringBuilder.toString(ReflectionToStringBuilder.java:288)
> at org.apache.commons.lang3.builder.ReflectionToStringBuilder.toString(ReflectionToStringBuilder.java:119)
> at com.seagullsw.appinterface.server.comm.BasicCommunicationManager.toString(BasicCommunicationManager.java:828)
> at com.seagullsw.appinterface.server.comm.BasicCommunicationManager.toString(BasicCommunicationManager.java:817)
> at java.lang.String.valueOf(String.java:2826)
> at java.lang.StringBuilder.append(StringBuilder.java:115)
> at com.seagullsw.appinterface.server.AisHelper.waitForCommuncationManagers(AisHelper.java:217)
> at com.seagullsw.appinterface.server.AisHelper.start(AisHelper.java:136)
> at com.seagullsw.appinterface.server.AisHelper.startFromResource(AisHelper.java:161)
> at com.seagullsw.appinterface.server.AbstractServerJunit4.startServer(AbstractServerJunit4.java:179)
> at com.seagullsw.appinterface.server.comm.jms.AbstractJmsRoundtripMaxConcurrencyTestCase.setUpOnce(AbstractJmsRoundtripMaxConcurrencyTestCase.java:141)
> at com.seagullsw.appinterface.server.comm.jms.ibmmq.JmsRoundtripMaxConcurrency032TestCase.setUpOnce(JmsRoundtripMaxConcurrency032TestCase.java:40)
> {noformat}
> We should either: 
> - Document ReflectionToStringBuilder and ToStringBuilder such that call sites use synchronized if the object passed in contains collections that are not thread-safe. F
> or example:
> {code:java}
>     @Override
>     public synchronized String toString() {
>         return ReflectionToStringBuilder.toString(this);
>     }
> {code}
> - Or have our code in ReflectionToStringBuilder and ToStringBuilder lock collections while they are being toString'd.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira