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

        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.
        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.
        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.
        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 -

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

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development