FeatureStoreRedis is not thread-safe

[kutzi]

When a method annotated with .@Flip is called concurrently from multiple threads, this very easily results in error getting the feature from the store.

E.g.
java.lang.ClassCastException: java.lang.Long cannot be cast to [B
at redis.clients.jedis.Connection.getBinaryBulkReply(Connection.java:216)
at redis.clients.jedis.Connection.getBulkReply(Connection.java:205)
at redis.clients.jedis.Jedis.get(Jedis.java:105)
at org.ff4j.store.FeatureStoreRedis.read(FeatureStoreRedis.java:136)
at org.ff4j.FF4j.getFeature(FF4j.java:465)
at org.ff4j.FF4j.check(FF4j.java:165)
at org.ff4j.aop.FeatureAdvisor.shouldFlip(FeatureAdvisor.java:202)
at org.ff4j.aop.FeatureAdvisor.invoke(FeatureAdvisor.java:79)

[kutzi]

This is because a single redisConnection is used to do everything, which is obviously not safe, when used from multiple threads.
I re-implemented the store by using a Spring RedisConnectionFactory backed by a Jedis pool:

[kutzi]

public class ThreadSafeFeatureStoreRedis extends AbstractFeatureStore {

  /** prefix of keys. */
  public static final String KEY_FEATURE = "FF4J_FEATURE_";

  /** default ttl. */
  private static int DEFAULT_TTL = 900000000;
  private final JedisConnectionFactory cf;

  /** time to live. */
  protected int timeToLive = DEFAULT_TTL;

  public ThreadSafeFeatureStoreRedis(JedisConnectionFactory cf) {
    this.cf = cf;
  }

  /** {@inheritDoc} */
  @Override
  public boolean exist(String uid) {
    Util.assertParamNotNull(uid, "Feature identifier");

    try (Jedis jedis = getJedis()) {
      return jedis.exists(KEY_FEATURE + uid);
    }
  }

  /** {@inheritDoc} */
  @Override
  public Feature read(String uid) {
    if (!exist(uid)) {
      throw new FeatureNotFoundException(uid);
    }

    try (Jedis jedis = getJedis()) {
      return FeatureJsonParser.parseFeature(jedis.get(KEY_FEATURE + uid));
    }
  }

  /** {@inheritDoc} */
  @Override
  public void update(Feature fp) {
    if (fp == null) {
      throw new IllegalArgumentException("Feature cannot be null");
    }
    if (!exist(fp.getUid())) {
      throw new FeatureNotFoundException(fp.getUid());
    }

    try (Jedis jedis = getJedis()) {
      jedis.set(KEY_FEATURE + fp.getUid(), fp.toJson());
      jedis.persist(KEY_FEATURE + fp.getUid());
    }
  }

  /** {@inheritDoc} */
  @Override
  public void enable(String uid) {
    // Read from redis, feature not found if no present
    Feature f = read(uid);
    // Update within Object
    f.enable();
    // Serialization and update key, update TTL
    update(f);
  }

  /** {@inheritDoc} */
  @Override
  public void disable(String uid) {
    // Read from redis, feature not found if no present
    Feature f = read(uid);
    // Update within Object
    f.disable();
    // Serialization and update key, update TTL
    update(f);
  }

  /** {@inheritDoc} */
  @Override
  public void create(Feature fp) {
    if (fp == null) {
      throw new IllegalArgumentException("Feature cannot be null nor empty");
    }
    if (exist(fp.getUid())) {
      throw new FeatureAlreadyExistException(fp.getUid());
    }

    try (Jedis jedis = getJedis()) {
      jedis.set(KEY_FEATURE + fp.getUid(), fp.toJson());
      jedis.persist(KEY_FEATURE + fp.getUid());
    }
  }

  /** {@inheritDoc} */
  @Override
  public Map<String, Feature> readAll() {
    Set< String > myKeys;
    try (Jedis jedis = getJedis()) {
      myKeys = jedis.keys(KEY_FEATURE + "*");
    }
    Map<String, Feature> myMap = new HashMap<String, Feature>();
    if (myKeys != null) {
      for (String key : myKeys) {
        key = key.replaceAll(KEY_FEATURE, "");
        myMap.put(key, read(key));
      }
    }
    return myMap;
  }

  /** {@inheritDoc} */
  @Override
  public void delete(String fpId) {
    if (!exist(fpId)) {
      throw new FeatureNotFoundException(fpId);
    }

    try (Jedis jedis = getJedis()) {
      jedis.del(KEY_FEATURE + fpId);
    }
  }

  /** {@inheritDoc} */
  @Override
  public void grantRoleOnFeature(String flipId, String roleName) {
    Util.assertParamNotNull(roleName, "roleName (#2)");
    // retrieve
    Feature f = read(flipId);
    // modify
    f.getPermissions().add(roleName);
    // persist modification
    update(f);
  }

  /** {@inheritDoc} */
  @Override
  public void removeRoleFromFeature(String flipId, String roleName) {
    Util.assertParamNotNull(roleName, "roleName (#2)");
    // retrieve
    Feature f = read(flipId);
    f.getPermissions().remove(roleName);
    // persist modification
    update(f);
  }

  /** {@inheritDoc} */
  @Override
  public Map<String, Feature> readGroup(String groupName) {
    Util.assertParamNotNull(groupName, "groupName");
    Map < String, Feature > features = readAll();
    Map < String, Feature > group = new HashMap<String, Feature>();
    for (Map.Entry<String,Feature> uid : features.entrySet()) {
      if (groupName.equals(uid.getValue().getGroup())) {
        group.put(uid.getKey(), uid.getValue());
      }
    }
    if (group.isEmpty()) {
      throw new GroupNotFoundException(groupName);
    }
    return group;
  }

  /** {@inheritDoc} */
  @Override
  public boolean existGroup(String groupName) {
    Util.assertParamNotNull(groupName, "groupName");
    Map < String, Feature > features = readAll();
    Map < String, Feature > group = new HashMap<String, Feature>();
    for (Map.Entry<String,Feature> uid : features.entrySet()) {
      if (groupName.equals(uid.getValue().getGroup())) {
        group.put(uid.getKey(), uid.getValue());
      }
    }
    return !group.isEmpty();
  }

  /** {@inheritDoc} */
  @Override
  public void enableGroup(String groupName) {
    Map < String, Feature > features = readGroup(groupName);
    for (Map.Entry<String,Feature> uid : features.entrySet()) {
      uid.getValue().enable();
      update(uid.getValue());
    }
  }

  /** {@inheritDoc} */
  @Override
  public void disableGroup(String groupName) {
    Map < String, Feature > features = readGroup(groupName);
    for (Map.Entry<String,Feature> uid : features.entrySet()) {
      uid.getValue().disable();
      update(uid.getValue());
    }
  }

  /** {@inheritDoc} */
  @Override
  public void addToGroup(String featureId, String groupName) {
    Util.assertParamNotNull(groupName, "groupName (#2)");
    // retrieve
    Feature f = read(featureId);
    f.setGroup(groupName);
    // persist modification
    update(f);
  }

  /** {@inheritDoc} */
  @Override
  public void removeFromGroup(String featureId, String groupName) {
    Util.assertParamNotNull(groupName, "groupName (#2)");
    if (!existGroup(groupName)) {
      throw new GroupNotFoundException(groupName);
    }
    // retrieve
    Feature f = read(featureId);
    f.setGroup(null);
    // persist modification
    update(f);
  }

  /** {@inheritDoc} */
  @Override
  public Set<String> readAllGroups() {
    Map < String, Feature > features = readAll();
    Set < String > groups = new HashSet<String>();
    for (Map.Entry<String,Feature> uid : features.entrySet()) {
      groups.add(uid.getValue().getGroup());
    }
    groups.remove(null);
    return groups;
  }


  /** {@inheritDoc} */
  @Override
  public void clear() {

    try (Jedis jedis = getJedis()) {
      Set<String> myKeys = jedis.keys(KEY_FEATURE + "*");
      jedis.del(myKeys.toArray(new String[0]));
    }
  }


  /**
   * Getter accessor for attribute 'timeToLive'.
   *
   * @return
   *       current value of 'timeToLive'
   */
  public int getTimeToLive() {
    return timeToLive;
  }

  /**
   * Setter accessor for attribute 'timeToLive'.
   * @param timeToLive
   *        new value for 'timeToLive '
   */
  public void setTimeToLive(int timeToLive) {
    this.timeToLive = timeToLive;
  }

  /**
   * Safe acces to Jedis, avoid JNPE.
   *
   * @return
   */
  public Jedis getJedis() {
    return cf.getConnection().getNativeConnection();
  }
}

[kutzi]

Same true for the PropertyStore and probably any other Redis based store there is

[clun]

I understand what you did. Thank you for that.

Still you introduced SpringData where there is no real added value.
I will take what you did and implement mutithreading with just the JedisPool.

I also have to allow connections through RedisSentinel. As a consequence I will probable create some interface JedisConnection provider

[kutzi]

I was not proposing to integrate SpringData in any way.
I just wanted to give an example how this could be fixed.

[kutzi]

It's just that we're using Spring already in our app. Therefore it was much easier to use JedisConnectionFactory directly

[clun]

Done, the JedisPool is now always enable to be thread safe. The Sentinel can also be enable ready to realease