Commons Lang
  1. Commons Lang
  2. LANG-329

Pointless synchronized in ThreadLocal.initialValue should be removed

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Trivial Trivial
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.4
    • Component/s: None
    • Labels:
      None
    • Environment:

      Any

      Description

      — jakarta/commons/proper/lang/trunk/src/java/org/apache/commons/lang/builder/ToStringStyle.java 2007/01/27 07:13:59 500497
      +++ jakarta/commons/proper/lang/trunk/src/java/org/apache/commons/lang/builder/ToStringStyle.java 2007/04/20 05:06:03 530645
      @@ -103,7 +103,7 @@

      • </p>
        */
        private static ThreadLocal registry = new ThreadLocal() {
      • protected synchronized Object initialValue() {
        + protected Object initialValue() {
        // The HashSet implementation is not synchronized,
        // which is just what we need here.
        return new HashSet();

      — jakarta/commons/proper/lang/trunk/src/java/org/apache/commons/lang/builder/HashCodeBuilder.java 2006/09/19 21:58:11 447989
      +++ jakarta/commons/proper/lang/trunk/src/java/org/apache/commons/lang/builder/HashCodeBuilder.java 2007/04/20 05:11:46 530648
      @@ -103,7 +103,7 @@

      • @since 2.3
        */
        private static ThreadLocal registry = new ThreadLocal() {
      • protected synchronized Object initialValue() {
        + protected Object initialValue() {
        // The HashSet implementation is not synchronized,
        // which is just what we need here.
        return new HashSet();

        Activity

        Transition Time In Source Status Execution Times Last Executer Last Execution Date
        Open Open Resolved Resolved
        1m 17s 1 Hanson Char 20/Apr/07 06:29
        Resolved Resolved Reopened Reopened
        110d 10h 23m 1 Henri Yandell 08/Aug/07 16:53
        Reopened Reopened Closed Closed
        10s 1 Henri Yandell 08/Aug/07 16:53
        Mark Thomas made changes -
        Workflow jira [ 12402248 ] Default workflow, editable Closed status [ 12602586 ]
        Henri Yandell made changes -
        Resolution Fixed [ 1 ]
        Fix Version/s 3.0 [ 12311714 ]
        Fix Version/s 2.3.1 [ 12312481 ]
        Status Reopened [ 4 ] Closed [ 6 ]
        Henri Yandell made changes -
        Status Resolved [ 5 ] Reopened [ 4 ]
        Resolution Fixed [ 1 ]
        Hide
        Hanson Char added a comment -

        %s/There is shared state/There is no shared state/g

        Show
        Hanson Char added a comment - %s/There is shared state/There is no shared state/g
        Hide
        Hanson Char added a comment -

        The reason is in this case the returned instance is a new instance every time. There is shared state. Synchronization is required only if a shared object is accessed, like the examples of read-modify-write integer increment they gave in the javadoc.

        Also, I've verified if this case is pointless or not in the jsr166 concurrency forum. See http://www.nabble.com/forum/ViewPost.jtp?post=10089323&framed=y

        Show
        Hanson Char added a comment - The reason is in this case the returned instance is a new instance every time. There is shared state. Synchronization is required only if a shared object is accessed, like the examples of read-modify-write integer increment they gave in the javadoc. Also, I've verified if this case is pointless or not in the jsr166 concurrency forum. See http://www.nabble.com/forum/ViewPost.jtp?post=10089323&framed=y
        Hide
        ggregory@seagullsw.com added a comment -

        Before we sign off on this I'd like to know why it is "pointless" and why does Sun recommend doing it the way the code was before this patch in their documentation.

        Please see:
        http://java.sun.com/j2se/1.4.2/docs/api/java/lang/ThreadLocal.html
        http://java.sun.com/j2se/1.5.0/docs/api/java/lang/ThreadLocal.html

        If you are on 1.6+, you'd do it differently:
        http://java.sun.com/javase/6/docs/api/java/lang/ThreadLocal.html

        Show
        ggregory@seagullsw.com added a comment - Before we sign off on this I'd like to know why it is "pointless" and why does Sun recommend doing it the way the code was before this patch in their documentation. Please see: http://java.sun.com/j2se/1.4.2/docs/api/java/lang/ThreadLocal.html http://java.sun.com/j2se/1.5.0/docs/api/java/lang/ThreadLocal.html If you are on 1.6+, you'd do it differently: http://java.sun.com/javase/6/docs/api/java/lang/ThreadLocal.html
        Hide
        Hanson Char added a comment -

        Will do the right thing next time. Thanks for the advise.

        Show
        Hanson Char added a comment - Will do the right thing next time. Thanks for the advise.
        Henri Yandell made changes -
        Fix Version/s 3.0 [ 12311714 ]
        Hide
        Henri Yandell added a comment -

        No major problems. Minor ones:

        • Set a fix version in JIRA to the next planned version. I'm doing that now (3.0).
        • Common to introduce yourself if people aren't expecting you to be committing to a component.
        • Also somewhat common I think to lag things between the report and the commit to allow for comments, though no biggy.

        This looks like a fine fix to me.

        Show
        Henri Yandell added a comment - No major problems. Minor ones: Set a fix version in JIRA to the next planned version. I'm doing that now (3.0). Common to introduce yourself if people aren't expecting you to be committing to a component. Also somewhat common I think to lag things between the report and the commit to allow for comments, though no biggy. This looks like a fine fix to me.
        Hanson Char made changes -
        Field Original Value New Value
        Status Open [ 1 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Hide
        Hanson Char added a comment -

        I've fixed it in the SVN main trunk. Let me know if any problem. Thanks.

        Show
        Hanson Char added a comment - I've fixed it in the SVN main trunk. Let me know if any problem. Thanks.
        Hanson Char created issue -

          People

          • Assignee:
            Unassigned
            Reporter:
            Hanson Char
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development