Thread-safe compiled expression evaluation in SpelExpression

Closes gh-24265

spring-expression/src/main/java/org/springframework/expression/spel/standard/SpelExpression.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2018 the original author or authors.
+ * Copyright 2002-2020 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -16,6 +16,8 @@
 
 package org.springframework.expression.spel.standard;
 
+import java.util.concurrent.atomic.AtomicInteger;
+
 import org.springframework.core.convert.TypeDescriptor;
 import org.springframework.expression.EvaluationContext;
 import org.springframework.expression.EvaluationException;
@@ -62,15 +64,15 @@ public class SpelExpression implements Expression {
 	private EvaluationContext evaluationContext;
 
 	// Holds the compiled form of the expression (if it has been compiled)
-	private CompiledExpression compiledAst;
+	private volatile CompiledExpression compiledAst;
 
 	// Count of many times as the expression been interpreted - can trigger compilation
 	// when certain limit reached
-	private volatile int interpretedCount = 0;
+	private final AtomicInteger interpretedCount = new AtomicInteger(0);
 
 	// The number of times compilation was attempted and failed - enables us to eventually
 	// give up trying to compile it when it just doesn't seem to be possible.
-	private volatile int failedAttempts = 0;
+	private final AtomicInteger failedAttempts = new AtomicInteger(0);
 
 
 	/**
@@ -112,16 +114,17 @@ public class SpelExpression implements Expression {
 
 	@Override
 	public Object getValue() throws EvaluationException {
-		if (this.compiledAst != null) {
+		CompiledExpression compiledAst = this.compiledAst;
+		if (compiledAst != null) {
 			try {
 				EvaluationContext context = getEvaluationContext();
-				return this.compiledAst.getValue(context.getRootObject().getValue(), context);
+				return compiledAst.getValue(context.getRootObject().getValue(), context);
 			}
 			catch (Throwable ex) {
 				// If running in mixed mode, revert to interpreted
 				if (this.configuration.getCompilerMode() == SpelCompilerMode.MIXED) {
-					this.interpretedCount = 0;
 					this.compiledAst = null;
+					this.interpretedCount.set(0);
 				}
 				else {
 					// Running in SpelCompilerMode.immediate mode - propagate exception to caller
@@ -139,10 +142,11 @@ public class SpelExpression implements Expression {
 	@SuppressWarnings("unchecked")
 	@Override
 	public <T> T getValue(Class<T> expectedResultType) throws EvaluationException {
-		if (this.compiledAst != null) {
+		CompiledExpression compiledAst = this.compiledAst;
+		if (compiledAst != null) {
 			try {
 				EvaluationContext context = getEvaluationContext();
-				Object result = this.compiledAst.getValue(context.getRootObject().getValue(), context);
+				Object result = compiledAst.getValue(context.getRootObject().getValue(), context);
 				if (expectedResultType == null) {
 					return (T) result;
 				}
@@ -154,8 +158,8 @@ public class SpelExpression implements Expression {
 			catch (Throwable ex) {
 				// If running in mixed mode, revert to interpreted
 				if (this.configuration.getCompilerMode() == SpelCompilerMode.MIXED) {
-					this.interpretedCount = 0;
 					this.compiledAst = null;
+					this.interpretedCount.set(0);
 				}
 				else {
 					// Running in SpelCompilerMode.immediate mode - propagate exception to caller
@@ -173,15 +177,16 @@ public class SpelExpression implements Expression {
 
 	@Override
 	public Object getValue(Object rootObject) throws EvaluationException {
-		if (this.compiledAst != null) {
+		CompiledExpression compiledAst = this.compiledAst;
+		if (compiledAst != null) {
 			try {
-				return this.compiledAst.getValue(rootObject, getEvaluationContext());
+				return compiledAst.getValue(rootObject, getEvaluationContext());
 			}
 			catch (Throwable ex) {
 				// If running in mixed mode, revert to interpreted
 				if (this.configuration.getCompilerMode() == SpelCompilerMode.MIXED) {
-					this.interpretedCount = 0;
 					this.compiledAst = null;
+					this.interpretedCount.set(0);
 				}
 				else {
 					// Running in SpelCompilerMode.immediate mode - propagate exception to caller
@@ -200,9 +205,10 @@ public class SpelExpression implements Expression {
 	@SuppressWarnings("unchecked")
 	@Override
 	public <T> T getValue(Object rootObject, Class<T> expectedResultType) throws EvaluationException {
-		if (this.compiledAst != null) {
+		CompiledExpression compiledAst = this.compiledAst;
+		if (compiledAst != null) {
 			try {
-				Object result = this.compiledAst.getValue(rootObject, getEvaluationContext());
+				Object result = compiledAst.getValue(rootObject, getEvaluationContext());
 				if (expectedResultType == null) {
 					return (T)result;
 				}
@@ -214,8 +220,8 @@ public class SpelExpression implements Expression {
 			catch (Throwable ex) {
 				// If running in mixed mode, revert to interpreted
 				if (this.configuration.getCompilerMode() == SpelCompilerMode.MIXED) {
-					this.interpretedCount = 0;
 					this.compiledAst = null;
+					this.interpretedCount.set(0);
 				}
 				else {
 					// Running in SpelCompilerMode.immediate mode - propagate exception to caller
@@ -236,15 +242,16 @@ public class SpelExpression implements Expression {
 	public Object getValue(EvaluationContext context) throws EvaluationException {
 		Assert.notNull(context, "EvaluationContext is required");
 
-		if (this.compiledAst != null) {
+		CompiledExpression compiledAst = this.compiledAst;
+		if (compiledAst != null) {
 			try {
-				return this.compiledAst.getValue(context.getRootObject().getValue(), context);
+				return compiledAst.getValue(context.getRootObject().getValue(), context);
 			}
 			catch (Throwable ex) {
 				// If running in mixed mode, revert to interpreted
 				if (this.configuration.getCompilerMode() == SpelCompilerMode.MIXED) {
-					this.interpretedCount = 0;
 					this.compiledAst = null;
+					this.interpretedCount.set(0);
 				}
 				else {
 					// Running in SpelCompilerMode.immediate mode - propagate exception to caller
@@ -264,9 +271,10 @@ public class SpelExpression implements Expression {
 	public <T> T getValue(EvaluationContext context, Class<T> expectedResultType) throws EvaluationException {
 		Assert.notNull(context, "EvaluationContext is required");
 
-		if (this.compiledAst != null) {
+		CompiledExpression compiledAst = this.compiledAst;
+		if (compiledAst != null) {
 			try {
-				Object result = this.compiledAst.getValue(context.getRootObject().getValue(), context);
+				Object result = compiledAst.getValue(context.getRootObject().getValue(), context);
 				if (expectedResultType != null) {
 					return ExpressionUtils.convertTypedValue(context, new TypedValue(result), expectedResultType);
 				}
@@ -277,8 +285,8 @@ public class SpelExpression implements Expression {
 			catch (Throwable ex) {
 				// If running in mixed mode, revert to interpreted
 				if (this.configuration.getCompilerMode() == SpelCompilerMode.MIXED) {
-					this.interpretedCount = 0;
 					this.compiledAst = null;
+					this.interpretedCount.set(0);
 				}
 				else {
 					// Running in SpelCompilerMode.immediate mode - propagate exception to caller
@@ -297,15 +305,16 @@ public class SpelExpression implements Expression {
 	public Object getValue(EvaluationContext context, Object rootObject) throws EvaluationException {
 		Assert.notNull(context, "EvaluationContext is required");
 
-		if (this.compiledAst != null) {
+		CompiledExpression compiledAst = this.compiledAst;
+		if (compiledAst != null) {
 			try {
-				return this.compiledAst.getValue(rootObject, context);
+				return compiledAst.getValue(rootObject, context);
 			}
 			catch (Throwable ex) {
 				// If running in mixed mode, revert to interpreted
 				if (this.configuration.getCompilerMode() == SpelCompilerMode.MIXED) {
-					this.interpretedCount = 0;
 					this.compiledAst = null;
+					this.interpretedCount.set(0);
 				}
 				else {
 					// Running in SpelCompilerMode.immediate mode - propagate exception to caller
@@ -327,9 +336,10 @@ public class SpelExpression implements Expression {
 
 		Assert.notNull(context, "EvaluationContext is required");
 
-		if (this.compiledAst != null) {
+		CompiledExpression compiledAst = this.compiledAst;
+		if (compiledAst != null) {
 			try {
-				Object result = this.compiledAst.getValue(rootObject, context);
+				Object result = compiledAst.getValue(rootObject, context);
 				if (expectedResultType != null) {
 					return ExpressionUtils.convertTypedValue(context, new TypedValue(result), expectedResultType);
 				}
@@ -340,8 +350,8 @@ public class SpelExpression implements Expression {
 			catch (Throwable ex) {
 				// If running in mixed mode, revert to interpreted
 				if (this.configuration.getCompilerMode() == SpelCompilerMode.MIXED) {
-					this.interpretedCount = 0;
 					this.compiledAst = null;
+					this.interpretedCount.set(0);
 				}
 				else {
 					// Running in SpelCompilerMode.immediate mode - propagate exception to caller
@@ -454,48 +464,58 @@ public class SpelExpression implements Expression {
 	 * @param expressionState the expression state used to determine compilation mode
 	 */
 	private void checkCompile(ExpressionState expressionState) {
-		this.interpretedCount++;
+		this.interpretedCount.incrementAndGet();
 		SpelCompilerMode compilerMode = expressionState.getConfiguration().getCompilerMode();
 		if (compilerMode != SpelCompilerMode.OFF) {
 			if (compilerMode == SpelCompilerMode.IMMEDIATE) {
-				if (this.interpretedCount > 1) {
+				if (this.interpretedCount.get() > 1) {
 					compileExpression();
 				}
 			}
 			else {
 				// compilerMode = SpelCompilerMode.MIXED
-				if (this.interpretedCount > INTERPRETED_COUNT_THRESHOLD) {
+				if (this.interpretedCount.get() > INTERPRETED_COUNT_THRESHOLD) {
 					compileExpression();
 				}
 			}
 		}
 	}
 
-
 	/**
-	 * Perform expression compilation. This will only succeed once exit descriptors for all nodes have
-	 * been determined. If the compilation fails and has failed more than 100 times the expression is
-	 * no longer considered suitable for compilation.
+	 * Perform expression compilation. This will only succeed once exit descriptors for
+	 * all nodes have been determined. If the compilation fails and has failed more than
+	 * 100 times the expression is no longer considered suitable for compilation.
+	 * @return whether this expression has been successfully compiled
 	 */
 	public boolean compileExpression() {
-		if (this.failedAttempts > FAILED_ATTEMPTS_THRESHOLD) {
+		CompiledExpression compiledAst = this.compiledAst;
+		if (compiledAst != null) {
+			// Previously compiled
+			return true;
+		}
+		if (this.failedAttempts.get() > FAILED_ATTEMPTS_THRESHOLD) {
 			// Don't try again
 			return false;
 		}
-		if (this.compiledAst == null) {
-			synchronized (this.expression) {
-				// Possibly compiled by another thread before this thread got into the sync block
-				if (this.compiledAst != null) {
-					return true;
-				}
-				SpelCompiler compiler = SpelCompiler.getCompiler(this.configuration.getCompilerClassLoader());
-				this.compiledAst = compiler.compile(this.ast);
-				if (this.compiledAst == null) {
-					this.failedAttempts++;
-				}
+
+		synchronized (this) {
+			if (this.compiledAst != null) {
+				// Compiled by another thread before this thread got into the sync block
+				return true;
+			}
+			SpelCompiler compiler = SpelCompiler.getCompiler(this.configuration.getCompilerClassLoader());
+			compiledAst = compiler.compile(this.ast);
+			if (compiledAst != null) {
+				// Successfully compiled
+				this.compiledAst = compiledAst;
+				return true;
+			}
+			else {
+				// Failed to compile
+				this.failedAttempts.incrementAndGet();
+				return false;
 			}
 		}
-		return (this.compiledAst != null);
 	}
 
 	/**
@@ -505,8 +525,8 @@ public class SpelExpression implements Expression {
 	 */
 	public void revertToInterpreted() {
 		this.compiledAst = null;
-		this.interpretedCount = 0;
-		this.failedAttempts = 0;
+		this.interpretedCount.set(0);
+		this.failedAttempts.set(0);
 	}
 
 	/**