Unsafe double-checked locking in SpelExpression#compileExpression

[PascalSchumacher]

The double-checked locking SpelExpression#compileExpression does not seem to be thread safe, because compiledAst is not volatile, see:

https://github.com/spring-projects/spring-framework/blob/master/spring-expression/src/main/java/org/springframework/expression/spel/standard/SpelExpression.java#L506

Or am I missing something?

Edit:
Also SpelExpression#compileExpression uses this.expression (a String) as a lock object. Shouldn't this be avoided, as Strings objects are shared throughout the program?

[sbrannen]

@aclement, can you please take a look at this?

[aclement]

Yep @PascalSchumacher I think you are right. Syncing on string has a chance to cause trouble (but I'm not aware of anyone reporting it actually did). Also, I think ideally we should make compiledAst volatile - although I don't think that causes us breaking issues other than not being optimal in this case (I am not an expert on java concurrency though!). We just might compile the same expression twice (the latest one winning), but It think there is no harm in that other than creating an unnecessary duplicate class representing the compiled expression, things shouldn't blow up.

Want to submit a PR?

[jhoeller]

@aclement @PascalSchumacher Thanks for the report and for the analysis! I'll take care of this, aligning it with similar arrangements in other parts of the codebase, and also backporting it. So no need for a PR, however, happy to take any further insight into account...

[PascalSchumacher]

@aclement @jhoeller Thank you very much for looking into this! (I do not have an CLA with Pivotal so I wasn't planning to submit a pull request.)