commons-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Woonsan Ko (JIRA)" <j...@apache.org>
Subject [jira] [Updated] (LANG-739) ToStringBuilder leaks memory if toString method causes hash code to be changed
Date Fri, 25 Oct 2013 16:16:34 GMT

     [ 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)

Mime
View raw message