jedis icon indicating copy to clipboard operation
jedis copied to clipboard

Fix jedis & pipeline has the same client result in thread issue

Open lionhuo opened this issue 3 years ago • 5 comments

Description

the pipeline has the same client with the jedis which get from jedis pool, if the jedis return to the pool, but the pipeline is still working, then the buf in RedisOutputStream maybe override, it will result in the wrong redis comand flush to redis server.

lionhuo avatar Oct 09 '21 11:10 lionhuo

@lionhuo It is expected that the user would be careful to avoid these situations. There several aspects/issues involved in handling this in Jedis, at least in current structure.

sazzad16 avatar Oct 11 '21 14:10 sazzad16

@sazzad16 It is expected that the user would be careful to avoid these situations. yes, you are right

but it may be cause some confusion, for me, execute command, hset {key} {filed} {value} got result: filed: "*1\r\n$4\r\nPING\r\nbalabalabala" of cause, it's a negative example

try(Jedis jedis = pool.getResouce()){
    Pipeline pip = jedis.pipelined(); //if pip escapes from the block, may cause issue
}

so i think the pipeline's lifecycle is independent maybe better, generate a hidden jedis object just service pipeline, release after pipeline finish(close/clear/sync/syncAndReturnAll)

Jedis.java

  @Override
  public Pipeline pipelined() {
    if (dataSource == null) {
      return super.pipelined();
    }
    
    try{
      Jedis jedis = this.dataSource.getResource();
      Pipeline pipeline = super.pipelined(jedis.client);
      pipeline.setJedis(jedis);
      return pipeline;
    }finally {
      this.close();
    }
  }

BinaryJedis.java

  public Pipeline pipelined(Client client) {
    pipeline = new Pipeline();
    pipeline.setClient(client);
    return pipeline;
  }

Pipeline.java

  public void sync() {
    try{
      if (getPipelinedResponseLength() > 0) {
        List<Object> unformatted = client.getMany(getPipelinedResponseLength());
        for (Object o : unformatted) {
          generateResponse(o);
        }
      }
    }finally {
      if(jedis != null){
        jedis.close();
      }
    }
  }

lionhuo avatar Oct 12 '21 03:10 lionhuo

@yangbodong22011

the pipeline is no longer available, this is incompatible with the previous semantics

ok, noted

i think Pipeline is the resource, it is the same with Jedis, so could we get it from JedisPool ? and release after call close method

try(Pipeline pip = pool.getPipeline()){
    ......
}

JedisPool.java

  public Pipeline getPipeline() {
    Jedis jedis = this.getResource();
    Pipeline pipeline = jedis.pipelined();
    pipeline.setJedis(jedis);
    return pipeline;
  }

Pipeline.java

@Override
  public void close() {
    try{
      clear();
    }finally {
      if(jedis != null){
        jedis.close();
      }
    }
  }

lionhuo avatar Oct 12 '21 08:10 lionhuo

try(Pipeline pip = pool.getPipeline()){ ...... }

I (personally) think we don’t need it, the following code is just one more line.

try(Jedis jedis = pool.getPipeline()){
    Pipeline pip = jedis.pipelined();
    ......
}

Let us wait for the opinions of others.

yangbodong22011 avatar Oct 14 '21 12:10 yangbodong22011

the following code is just one more line.

you are right

but i think get Pipeline from the pool,It reinforces Pipeline is a independent resource of Jedis and user don't need careful to use Jedis and Pipeline if Pipeline escapes from Jedis block

lionhuo avatar Oct 15 '21 06:10 lionhuo