groovy

@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

Ascending order - Click to sort in descending order
Hide
Permalink
Paul King added a comment - - edited

Hi, the code is as designed - the "isVolatile" method has the wrong name by accident - it functions correctly but should have exactly the opposite name (ouch!). I have renamed it to "notVolatile" which is what it does. Thanks for noticing this ugly "typo".

If you leave out "volatile" you will indeed get a non-thread safe lazy implementation by design for those use cases where you want the simplest possible lazy instantation of your field and know that it won't be used in a multi-threaded environment. Always use volatile when you want the thread-safe version.

Show
Paul King added a comment - - edited Hi, the code is as designed - the "isVolatile" method has the wrong name by accident - it functions correctly but should have exactly the opposite name (ouch!). I have renamed it to "notVolatile" which is what it does. Thanks for noticing this ugly "typo". If you leave out "volatile" you will indeed get a non-thread safe lazy implementation by design for those use cases where you want the simplest possible lazy instantation of your field and know that it won't be used in a multi-threaded environment. Always use volatile when you want the thread-safe version.
Hide
Permalink
thurston n added a comment -

My bad. I did look at isVolatile and obviously it didn't register.
It does point out that FieldNode should be refactored to be more OO; isVolatile(), isFinal(), etc. should be added directly to the FieldNode class itself (modifiers can still be left exposed of course). Not doing so just invites a proliferation of little is(Not)Volatile methods, and all the attendant trouble

And although the choice you made about interpreting the source's use of volatile is certainly reasonable, it might be helpful to add that explicitly to the JavaDoc; I'm just thinking about all the dead bytes that have been spent on explaining DCL. It's probably asking too much to expect users to look at the bytecode (I did and look where it got me )

Show
thurston n added a comment - My bad. I did look at isVolatile and obviously it didn't register. It does point out that FieldNode should be refactored to be more OO; isVolatile(), isFinal(), etc. should be added directly to the FieldNode class itself (modifiers can still be left exposed of course). Not doing so just invites a proliferation of little is(Not)Volatile methods, and all the attendant trouble And although the choice you made about interpreting the source's use of volatile is certainly reasonable, it might be helpful to add that explicitly to the JavaDoc; I'm just thinking about all the dead bytes that have been spent on explaining DCL. It's probably asking too much to expect users to look at the bytecode (I did and look where it got me )
Hide
Permalink
blackdrag blackdrag added a comment -

normally there is no need for an extra isVolatile method, since the Java class Modifier provides methods for that. We use exactly the same modifiers as Java does, so it is no problem to reuse that class

Show
blackdrag blackdrag added a comment - normally there is no need for an extra isVolatile method, since the Java class Modifier provides methods for that. We use exactly the same modifiers as Java does, so it is no problem to reuse that class
Hide
Permalink
Paul King added a comment -

This issue is already closed, so just a Post-Mortem comment. FieldNode already has isFinal() - and now in 2.0 has isVolatile() - as a general rule though, given that Modifier#isVolatile() already exists, we tend to only push such functionality into classes like FieldNode if it is widely enough used that the shortcut adds a lot to code clarity - in this instance I think it is only marginally clearer - but it certainly is a little more consistent.

Show
Paul King added a comment - This issue is already closed, so just a Post-Mortem comment. FieldNode already has isFinal() - and now in 2.0 has isVolatile() - as a general rule though, given that Modifier#isVolatile() already exists, we tend to only push such functionality into classes like FieldNode if it is widely enough used that the shortcut adds a lot to code clarity - in this instance I think it is only marginally clearer - but it certainly is a little more consistent.
Hide
Permalink
thurston n added a comment -

Sorry, I tried posting this to groovy-dev mailing list but it got rejected (even though I successfully subscribed via xircles):

As part of a @Lazy thread-safety discussion, it got me to thinking, "how would you test this?".
As far as I can tell, the tests of ASTTransformations rely on shell.evaluate("""source"""), but that's not going to cut it in the case of the above issue.
I'm assuming at some point you can access the entire AST before it gets "written" and then you could walk the AST tree, but it sure seems like that would lead to some very brittle tests (you wouldn't want every compiler optimization to produce a slew of test failures).
So how ideally would you go about testing similar issues?

In a somewhat related vein, apropos of Jochen's work on invokedynamic support, how is that planning to be integrated in a common codebase that needs to support < 1.7? Will it be an argument to groovyc? or will groovyc rely on runtime detection of java.version? I'm assuming that the bytecode itself will not do runtime detection (on initial thought that would be a nightmare)

Show
thurston n added a comment - Sorry, I tried posting this to groovy-dev mailing list but it got rejected (even though I successfully subscribed via xircles): As part of a @Lazy thread-safety discussion, it got me to thinking, "how would you test this?". As far as I can tell, the tests of ASTTransformations rely on shell.evaluate("""source"""), but that's not going to cut it in the case of the above issue. I'm assuming at some point you can access the entire AST before it gets "written" and then you could walk the AST tree, but it sure seems like that would lead to some very brittle tests (you wouldn't want every compiler optimization to produce a slew of test failures). So how ideally would you go about testing similar issues? In a somewhat related vein, apropos of Jochen's work on invokedynamic support, how is that planning to be integrated in a common codebase that needs to support < 1.7? Will it be an argument to groovyc? or will groovyc rely on runtime detection of java.version? I'm assuming that the bytecode itself will not do runtime detection (on initial thought that would be a nightmare)
Hide
Permalink
blackdrag blackdrag added a comment -

We intend to make two distributions, one which has support for indy, the other not. But basically you will have an option "useIndy" for this.

Show
blackdrag blackdrag added a comment - We intend to make two distributions, one which has support for indy, the other not. But basically you will have an option "useIndy" for this.

People

  • Assignee:
    Paul King
    Reporter:
    thurston n
Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: