Skip to content

Commit

Permalink
Prevent recursion from Response.build() with dependency
Browse files Browse the repository at this point in the history
* Fixes #856
* Before patch it, users could experience StackOverflowError
** when there's multi in Pipeline, which has massive requests

Conflicts:
	src/test/java/redis/clients/jedis/tests/PipeliningTest.java
  • Loading branch information
HeartSaVioR authored and marcosnils committed Feb 4, 2015
1 parent 7274a00 commit 8216735
Show file tree
Hide file tree
Showing 2 changed files with 49 additions and 10 deletions.
36 changes: 26 additions & 10 deletions src/main/java/redis/clients/jedis/Response.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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) {
Expand All @@ -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() {
Expand Down
23 changes: 23 additions & 0 deletions src/test/java/redis/clients/jedis/tests/PipeliningTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,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");
Expand Down

0 comments on commit 8216735

Please sign in to comment.