From 4fa992c483344b50cd49973d87bc56b41140d1c8 Mon Sep 17 00:00:00 2001
From: Denis Zavedeev <happy96538@gmail.com>
Date: Thu, 12 Sep 2019 20:43:55 +0300
Subject: [PATCH] DATAREDIS-1034 - Fix dataraces in RedisTemplate.

Previously it was possible in multi-threaded environment to:
* get a `null` from `opsFor*` methods in RedisTemplate.
* fail with a `NullPointerException` when working with a not-`null` reference returned from `RedisTemplate.opsFor*` methods.

Introduced changes initialize the fields eagerly to avoid a datarace when accessing them and add a `final` modifier to the `AbstractOperations#template` field.

Please note that these changes still require a client to publish a reference to RedisTemplate safely (getting a reference from an `ApplicationContext` is fine).
Also note that escaping a reference to `this` in `RedisTemplate` initializer should also be ok, because putting a bean into `ApplicationContext` happens-before its subsequent retrieval.

Original pull request: #479.
---
 .../data/redis/core/AbstractOperations.java   |  3 +-
 .../data/redis/core/RedisTemplate.java        | 47 ++++---------------
 2 files changed, 12 insertions(+), 38 deletions(-)

diff --git a/src/main/java/org/springframework/data/redis/core/AbstractOperations.java b/src/main/java/org/springframework/data/redis/core/AbstractOperations.java
index dfb2520ed8..3baaa548f8 100644
--- a/src/main/java/org/springframework/data/redis/core/AbstractOperations.java
+++ b/src/main/java/org/springframework/data/redis/core/AbstractOperations.java
@@ -44,6 +44,7 @@
  * @author Christoph Strobl
  * @author David Liu
  * @author Mark Paluch
+ * @author Denis Zavedeev
  */
 abstract class AbstractOperations<K, V> {
 
@@ -64,7 +65,7 @@ public final V doInRedis(RedisConnection connection) {
 		protected abstract byte[] inRedis(byte[] rawKey, RedisConnection connection);
 	}
 
-	RedisTemplate<K, V> template;
+	final RedisTemplate<K, V> template;
 
 	AbstractOperations(RedisTemplate<K, V> template) {
 		this.template = template;
diff --git a/src/main/java/org/springframework/data/redis/core/RedisTemplate.java b/src/main/java/org/springframework/data/redis/core/RedisTemplate.java
index a1a85fe8df..540b6ebf45 100644
--- a/src/main/java/org/springframework/data/redis/core/RedisTemplate.java
+++ b/src/main/java/org/springframework/data/redis/core/RedisTemplate.java
@@ -81,6 +81,7 @@
  * @author Ninad Divadkar
  * @author Anqing Shao
  * @author Mark Paluch
+ * @author Denis Zavedeev
  * @param <K> the Redis key type against which the template works (usually a String)
  * @param <V> the Redis value type against which the template works
  * @see StringRedisTemplate
@@ -102,14 +103,14 @@ public class RedisTemplate<K, V> extends RedisAccessor implements RedisOperation
 
 	private @Nullable ScriptExecutor<K> scriptExecutor;
 
-	// cache singleton objects (where possible)
-	private @Nullable ValueOperations<K, V> valueOps;
-	private @Nullable ListOperations<K, V> listOps;
-	private @Nullable SetOperations<K, V> setOps;
-	private @Nullable StreamOperations<K, ?, ?> streamOps;
-	private @Nullable ZSetOperations<K, V> zSetOps;
-	private @Nullable GeoOperations<K, V> geoOps;
-	private @Nullable HyperLogLogOperations<K, V> hllOps;
+	private final ValueOperations<K, V> valueOps = new DefaultValueOperations<>(this);
+	private final ListOperations<K, V> listOps = new DefaultListOperations<>(this);
+	private final SetOperations<K, V> setOps = new DefaultSetOperations<>(this);
+	private final StreamOperations<K, ?, ?> streamOps = new DefaultStreamOperations<>(this, new ObjectHashMapper());
+	private final ZSetOperations<K, V> zSetOps = new DefaultZSetOperations<>(this);
+	private final GeoOperations<K, V> geoOps = new DefaultGeoOperations<>(this);
+	private final HyperLogLogOperations<K, V> hllOps = new DefaultHyperLogLogOperations<>(this);
+	private final ClusterOperations<K, V> clusterOps = new DefaultClusterOperations<>(this);
 
 	/**
 	 * Constructs a new <code>RedisTemplate</code> instance.
@@ -1203,7 +1204,7 @@ public void slaveOfNoOne() {
 	 */
 	@Override
 	public ClusterOperations<K, V> opsForCluster() {
-		return new DefaultClusterOperations<>(this);
+		return clusterOps;
 	}
 
 	/*
@@ -1212,10 +1213,6 @@ public ClusterOperations<K, V> opsForCluster() {
 	 */
 	@Override
 	public GeoOperations<K, V> opsForGeo() {
-
-		if (geoOps == null) {
-			geoOps = new DefaultGeoOperations<>(this);
-		}
 		return geoOps;
 	}
 
@@ -1252,10 +1249,6 @@ public <HK, HV> HashOperations<K, HK, HV> opsForHash() {
 	 */
 	@Override
 	public HyperLogLogOperations<K, V> opsForHyperLogLog() {
-
-		if (hllOps == null) {
-			hllOps = new DefaultHyperLogLogOperations<>(this);
-		}
 		return hllOps;
 	}
 
@@ -1265,10 +1258,6 @@ public HyperLogLogOperations<K, V> opsForHyperLogLog() {
 	 */
 	@Override
 	public ListOperations<K, V> opsForList() {
-
-		if (listOps == null) {
-			listOps = new DefaultListOperations<>(this);
-		}
 		return listOps;
 	}
 
@@ -1296,10 +1285,6 @@ public BoundSetOperations<K, V> boundSetOps(K key) {
 	 */
 	@Override
 	public SetOperations<K, V> opsForSet() {
-
-		if (setOps == null) {
-			setOps = new DefaultSetOperations<>(this);
-		}
 		return setOps;
 	}
 
@@ -1309,10 +1294,6 @@ public SetOperations<K, V> opsForSet() {
 	 */
 	@Override
 	public <HK, HV> StreamOperations<K, HK, HV> opsForStream() {
-
-		if (streamOps == null) {
-			streamOps = new DefaultStreamOperations<>(this, new ObjectHashMapper());
-		}
 		return (StreamOperations<K, HK, HV>) streamOps;
 	}
 
@@ -1350,10 +1331,6 @@ public BoundValueOperations<K, V> boundValueOps(K key) {
 	 */
 	@Override
 	public ValueOperations<K, V> opsForValue() {
-
-		if (valueOps == null) {
-			valueOps = new DefaultValueOperations<>(this);
-		}
 		return valueOps;
 	}
 
@@ -1372,10 +1349,6 @@ public BoundZSetOperations<K, V> boundZSetOps(K key) {
 	 */
 	@Override
 	public ZSetOperations<K, V> opsForZSet() {
-
-		if (zSetOps == null) {
-			zSetOps = new DefaultZSetOperations<>(this);
-		}
 		return zSetOps;
 	}