[GEODE-10121] Transactional Redis commands are not actually transactional

• Type: Bug
• Status: Closed
• Priority: Major
• Resolution: Fixed
• Affects Version/s: 1.15.0
• Fix Version/s: 1.15.0
• Component/s: redis
• Labels: pull-request-available redis-triage
• Assignee: Bala Tripura Sundari Kaza Venkata
• Reporter: Donal Evans
• Created: 12/Mar/22 00:11
• Updated: 22/Jun/22 20:46
• Resolved: 06/Apr/22 15:21

Description

The following test (if added to MSetDUnitTest) is intended to show that MSET is transactional in nature by having the final key to be set throw an exception, which should roll back the previous set values:

  public static final String THROWING_REDIS_STRING_EXCEPTION = "to be ignored";
  
  @Test
  public void mset_isTransactional() {
    IgnoredException.addIgnoredException(THROWING_REDIS_STRING_EXCEPTION);
    String hashTag = "{" + clusterStartUp.getKeyOnServer("tag", 1) + "}";

    String key1 = hashTag + "key1";
    String value1 = "value1";
    jedis.set(key1, value1);
    String key2 = hashTag + "key2";
    String value2 = "value2";
    jedis.set(key2, value2);

    String throwingRedisStringKey = hashTag + "ThrowingRedisString";
    String throwingStringValue = "ThrowingRedisStringValue";

    // Put a test version of RedisString directly into the region that throws if the multi-argument set() is called on it
    clusterStartUp.getMember(1).invoke(() -> {
      RedisKey throwingKey = new RedisKey(throwingRedisStringKey.getBytes(StandardCharsets.UTF_8));
      ThrowingRedisString throwingRedisString = new ThrowingRedisString();
      throwingRedisString.set(throwingStringValue.getBytes(StandardCharsets.UTF_8));
      ClusterStartupRule.getCache().getRegion(DEFAULT_REDIS_REGION_NAME).put(throwingKey, throwingRedisString);
    });

    String newValue = "should_not_be_set";

    assertThatThrownBy(() -> jedis.mset(key1, newValue, key2, newValue, throwingRedisStringKey, newValue)).hasMessage(SERVER_ERROR_MESSAGE);
    
    assertThat(jedis.get(key1)).isEqualTo(value1);
    assertThat(jedis.get(key2)).isEqualTo(value2);
    assertThat(jedis.get(throwingRedisStringKey)).isEqualTo(throwingStringValue);

    IgnoredException.removeAllExpectedExceptions();
  }

  static class ThrowingRedisString extends RedisString {
    @Override
    public void set(Region<RedisKey, RedisData> region, RedisKey key, byte[] newValue,
                    SetOptions options) {
      throw new RuntimeException(THROWING_REDIS_STRING_EXCEPTION);
    }
  }

This test fails due to key1 having its value set to newValue despite the fact that MSET is supposed to be atomic.

Given the similar implementations each command uses, it is very likely that MSETNX and SMOVE also show the same behaviour.

Issue Links

• links to: GitHub Pull Request #7513