Merge pull request #860 from HeartSaVioR/not-allowing-build-recursion…

…-on-pipeline-with-multi Prevent recursion from Response.build() with dependency (Fixes #856)
redis · Feb 4, 2015 · dc9ba6d · dc9ba6d
2 parents b6f06ef + f4bb295
commit dc9ba6d
Show file tree

Hide file tree

Showing 2 changed files with 50 additions and 17 deletions.
diff --git a/src/main/java/redis/clients/jedis/Response.java b/src/main/java/redis/clients/jedis/Response.java
@@ -4,12 +4,15 @@
 
 public class Response<T> {
   protected T response = null;
+  protected JedisDataException exception = null;
+
+  private boolean building = false;
   private boolean built = false;
   private boolean set = false;
+
   private Builder<T> builder;
   private Object data;
   private Response<?> dependency = null;
-  private boolean requestDependencyBuild = false;
 
   public Response(Builder<T> b) {
     this.builder = b;
@@ -23,8 +26,7 @@ public void set(Object data) {
   public T get() {
     // if response has dependency response and dependency is not built,
     // build it first and no more!!
-    if (!requestDependencyBuild && dependency != null && dependency.set && !dependency.built) {
-      requestDependencyBuild = true;
+    if (dependency != null && dependency.set && !dependency.built) {
       dependency.build();
     }
     if (!set) {
@@ -34,23 +36,37 @@ public T get() {
     if (!built) {
       build();
     }
+    if (exception != null) {
+      throw exception;
+    }
     return response;
   }
 
   public void setDependency(Response<?> dependency) {
     this.dependency = dependency;
-    this.requestDependencyBuild = false;
   }
 
   private void build() {
-    if (data != null) {
-      if (data instanceof JedisDataException) {
-        throw new JedisDataException((JedisDataException) data);
+    // check build state to prevent recursion
+    if (building) {
+      return;
+    }
+
+    building = true;
+    try {
+      if (data != null) {
+        if (data instanceof JedisDataException) {
+          exception = (JedisDataException) data;
+        } else {
+          response = builder.build(data);
+        }
       }
-      response = builder.build(data);
+
+      data = null;
+    } finally {
+      building = false;
+      built = true;
     }
-    data = null;
-    built = true;
   }
 
   public String toString() {

diff --git a/src/test/java/redis/clients/jedis/tests/PipeliningTest.java b/src/test/java/redis/clients/jedis/tests/PipeliningTest.java
@@ -1,13 +1,7 @@
 package redis.clients.jedis.tests;
 
 import java.io.UnsupportedEncodingException;
-import java.util.Arrays;
-import java.util.HashMap;
-import java.util.Iterator;
-import java.util.List;
-import java.util.Map;
-import java.util.Set;
-import java.util.UUID;
+import java.util.*;
 
 import org.hamcrest.CoreMatchers;
 import org.hamcrest.Matcher;
@@ -255,6 +249,29 @@ public void multi() {
 
   }
 
+  @Test
+  public void multiWithMassiveRequests() {
+    Pipeline p = jedis.pipelined();
+    p.multi();
+
+    List<Response<?>> responseList = new ArrayList<Response<?>>();
+    for (int i = 0; i < 100000; i++) {
+      // any operation should be ok, but shouldn't forget about timeout
+      responseList.add(p.setbit("test", 1, true));
+    }
+
+    Response<List<Object>> exec = p.exec();
+    p.sync();
+
+    // we don't need to check return value
+    // if below codes run without throwing Exception, we're ok
+    exec.get();
+
+    for (Response<?> resp : responseList) {
+      resp.get();
+    }
+  }
+
   @Test
   public void multiWithSync() {
     jedis.set("foo", "314");