groovy
  1. groovy
  2. GROOVY-5316

@Lazy (LazyASTTransformation) not multi-thread safe

    Details

    • Type: Bug Bug
    • Status: Closed Closed
    • Priority: Minor Minor
    • Resolution: Not A Bug
    • Affects Version/s: 2.0-beta-2
    • Fix Version/s: 2.0-beta-3, 1.8.7
    • Component/s: Compiler
    • Labels:
      None
    • Number of attachments :
      0

      Description

      I checked out the latest code from git on the master branch, and looking at LazyASTTransformation, it doesn't appear to be multi-thread safe in the case of an instance variable annotated with @Lazy.
      specifically:

      private void create(FieldNode fieldNode, final Expression initExpr) {
              final BlockStatement body = new BlockStatement();
              if (fieldNode.isStatic()) {
                  addHolderClassIdiomBody(body, fieldNode, initExpr);
              } else if (isVolatile(fieldNode)) {
                  addNonThreadSafeBody(body, fieldNode, initExpr);
              } else {
                  addDoubleCheckedLockingBody(body, fieldNode, initExpr);
              }
              ...
      

      But the JMM makes DCL-locking work with volatile, i.e. it doesn't obviate the need for DCL in the case of volatile instance variables. As written now, it seems the resulting code isn't multi-thread safe in either case (the source declares the field volatile or not)
      Interestingly enough, @Singleton("lazy") (SingletonASTTransformation) does this correctly, by declaring a volatile variable (named "instance", although static) and then applying the DCL idiom (see #GROOVY-4782).

      So it looks like the issue is understood but somehow isn't in @Lazy.
      The fix seems rather straightforward:
      modify the @Lazy FieldNode to always be volatile (whether declared volatile or not in the source)
      and then create becomes:

      private void create(FieldNode fieldNode, final Expression initExpr) {
              final BlockStatement body = new BlockStatement();
              if (fieldNode.isStatic()) {
                  addHolderClassIdiomBody(body, fieldNode, initExpr);
              else
                  addDoubleCheckedLockingBody(body, fieldNode, initExpr);
              ...        
      

        Activity

        No work has yet been logged on this issue.

          People

          • Assignee:
            Paul King
            Reporter:
            thurston n
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: