Mailing-List: contact issues-help@commons.apache.org; run by ezmlm
Precedence: bulk
Reply-To: issues@commons.apache.org
Date: Fri, 25 Oct 2013 16:16:34 +0000 (UTC)
From: "Woonsan Ko (JIRA)" <jira@apache.org>
To: issues@commons.apache.org
Message-ID: <JIRA.12518333.1312908472207.2581.1382717794337@arcas>
In-Reply-To: <JIRA.12518333.1312908472207@arcas>
References: <JIRA.12518333.1312908472207@arcas>
Subject: [jira] [Updated] (LANG-739) ToStringBuilder leaks memory if
 toString method causes hash code to be changed
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit


     [ https://issues.apache.org/jira/browse/LANG-739?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Woonsan Ko updated LANG-739:
----------------------------

    Attachment: LANG-739-patch2.txt

Thanks a lot for the quick review, Philippe!
Indeed, the risk with WeakHashMap doesn't seem to be necessary.

So, I'm attaching a new patch (LANG-739-patch2.txt), with which I replaced WeakHashMap by HashMap.
I think this is safe because a) ToStringStyle#appendInternal() does register/unregister in try ~ finally block, and b) ToStringStyle#appendStart() does registration and ToStringStyle#appendEnd() should follow and do the unregistration (like ToStringBuilder#reflectionToString() does this).
Also, to show the paired registration/unregistration more clearly in b), I moved the scattered register() calls to #appendStart().

Please take a review again.

Cheers,

Woonsan

> ToStringBuilder leaks memory if toString method causes hash code to be changed
> ------------------------------------------------------------------------------
>
>                 Key: LANG-739
>                 URL: https://issues.apache.org/jira/browse/LANG-739
>             Project: Commons Lang
>          Issue Type: Bug
>          Components: lang.builder.*
>    Affects Versions: 2.3
>            Reporter: Philippe Renon
>             Fix For: Review Patch
>
>         Attachments: LANG-739-patch2.txt, LANG-739-patch.txt
>
>
> We have the following abstract class:
> {code}
> public class AbstractMessageItem {
>     private String toString;
>     public boolean equals(final Object obj) {
>         return EqualsBuilder.reflectionEquals(this, obj);
>     }
>     public int hashCode() {
>         return HashCodeBuilder.reflectionHashCode(this);
>     }
>     public String toString() {
>         if (toString == null) {
>             toString = ToStringBuilder.reflectionToString(this);
>         }
>         return toString;
>     }
> }
> {code}
> We also have two concrete classes extending the above class and one of them has a reference to the other.
> Now, if we call toString() on the 1st one, this will in turn call toString() on the second one.
> The call to toString() on the second one will cause its hash code to be changed and as a consequence will also change the hashCode of the first one *while* computing its toString().
> This causes the _infinite loop avoidance_ mechanism (i.e. the registry) to fail to unregister some objects and memory will be leaked.
> I believe that this leak can be avoided by using the system identity hash code when registering objects (as is done in HashCodeBuilder) instead of the user hash code.
> I know the issue can be worked around by removing the toString field (and loosing a dubious "performance enhancement" hack) or by making it transient, but I think that other "mutating" toString() methods can happen in the field (sometimes for good reasons) and fixing ToStringBuilder can be of help in some cases.


--
This message was sent by Atlassian JIRA
(v6.1#6144)