Writing to read-only slave fails silently inside of MULTI block

[aniketschneider]

Expected behavior

When writing to a read-only slave node outside of a transaction throws a JedisDataException. I would expect the same behavior inside of a transaction, as seen in the command-line redis client:

127.0.0.1:7379> multi
OK
127.0.0.1:7379> set test test
(error) READONLY You can't write against a read only slave.
127.0.0.1:7379> exec
(empty list or set)

Actual behavior

Writing to a read-only slave as part of a transaction fails silently.

Steps to reproduce:

This fails to write anything without producing any errors:

Jedis j = new Jedis("read-only-slave");

Transaction t = j.multi();
t.set("test", "test");
List<Object> results = t.exec();

Redis / Jedis Configuration

Jedis version: 2.9.0

Redis version: 2.8.13, 3.2.8

Java version: 1.8.0_77

[marcosnils]

Works as expected

Check https://github.com/xetorthio/jedis/blob/1f1e23b26a4aa244ebfdbcbcdf489b630c841884/src/test/java/redis/clients/jedis/tests/commands/TransactionCommandsTest.java#L218-L232

[aniketschneider]

I don't think your test is describing the same situation. If create a transaction against a read-only database with a single write command and no reads, there is no error response. This test fails:

  @Test
  public void testReadOnlyWrite() {
    Jedis j = new Jedis("localhost", 7379);

    Transaction t = j.multi();
    Response<String> resp = t.set("foo", "bar");
    List<Object> results = t.exec();

    assertEquals(results.size(), 1);
  }

[marcosnils]

@sheinbergon @HeartSaVioR can you give me a hand with this?. I'm super bust ATM :cry:

[sheinbergon]

OK. so @marcosnils @aniketschneider this is actually quite weird. I'm able to reproduce this issue, but only with a single write and a single read. There are several issues here

1. The exception exception error message is misleading/wrong. We should probably clarify it to make more sense.

2. The error handling logic is based on the fact the the set flag is not raised inside the Response class. That flag is only raised assuming some response is received. I think it makes sense, but it's worth discussing.

3. The problem is inside Queable , specifically in :

 protected Response<?> generateResponse(Object data) {
    Response<?> response = pipelinedResponses.poll();
    if (response != null) {
      response.set(data);
    }
    return response;
  }

It tests if the whole object is null, where in-fact, we're facing a scenario where the response body/content is null. So the first fix ( and a one that would make this functionality work as expected )
would be to change this test

4. The pipelineResponses field in this object contains only a single response object with its response/exception fields set to null. We of course expect to see the original ERROR message in response. Sniffing the traffic via tcpdump, you see this :

01:46:06.021491 IP 127.0.0.1.56202 > 127.0.0.1.7379: Flags [P.], seq 16:69, ack 6, win 342, options [nop,nop,TS val 935806 ecr 935803], length 53
E..i..@[email protected].............{.O....y...V.].....
..G~..G{*3
$3
SET
$3
noo
$3
bar
*2
$3
GET
$3
foo

01:46:06.021549 IP 127.0.0.1.7379 > 127.0.0.1.56202: Flags [P.], seq 6:69, ack 69, win 342, options [nop,nop,TS val 935806 ecr 935806], length 63
E..s..@[email protected]{.P)...V.g.....
..G~..G~-READONLY You can't write against a read only slave.
+QUEUED

01:46:06.021768 IP 127.0.0.1.56202 > 127.0.0.1.7379: Flags [P.], seq 69:83, ack 69, win 342, options [nop,nop,TS val 935806 ecr 935806], length 14
E..B..@[email protected].............{.P).......V.6.....
..G~..G~*1
$4
EXEC

So to make a long story short , The 'read-only' error takes place even before the exec and therefor not caught by jedis, only some weird null response is returned. To my understanding, that means that it's not something jedis can cope on it's own and we should just stick with what I suggested in section 3.

Hope that's thorough enough :)

[sheinbergon]

@marcosnils I'll be happy to write a PR to fix this. Let me know.

[aniketschneider]

@sheinbergon I appreciate you digging into this! I am following along as best I can.

I'm not sure if this is helpful or not, but when following the execution of this test through the jedis code, I ended up here: https://github.com/xetorthio/jedis/blob/master/src/main/java/redis/clients/jedis/Transaction.java#L43

This getMany call does in fact return the relevant JedisDataExceptions caused by the READONLY status of the node, but they are being intentionally discarded.

From my naive perspective, if it's not possible to throw an exception or produce an error when the write command is initially issued (before the EXEC), I would like to see an exception here (before the EXEC is actually processed) if there are any errors in the list returned by getMany. After all, if some of the commands have not succeeded, I don't want the rest of the transaction to be committed. However, I don't know if this makes sense for other situations where an error might be included in that list. After all, if this only comes up when a node is a readonly slave, then it doesn't matter if the transaction is committed or not because the transaction cannot make any changes to the data.

[sheinbergon]

@aniketschneider Yes the point that this error takes place before the exec is what makes the handling more difficult, I came to the same conclusion. The fix suggested in paragraph 3 will eliminate this issue.

Regarding the 2nd part of your comment, I'm not sure that's how transactions work. Conceptually, the whole point of a transaction is performing an "atomic" block - either everything succeeds or nothing happens. This is not a matter of "SDK implementation behavior", but rather how Redis itself behaves.

In any case an internal Redis error is raised, the transaction shouldn't complete, and roll back ( if there's something to rollback at all). again, that's not up to the SDK :) so I believe that from that perspective, you're covered.

@marcosnils what about commenting on my fix suggestion above?

[marcosnils]

@sheinbergon your proposed fix seems right, would really appreciate your help crafting a PR.

[sheinbergon]

@marcosnils my suggested fix was not good enough. It seems that there are some situations where Object data returned is expected to be null ( say some of the tests describing eval usage scenarios, or even in general - if eval was used inside multi-exec/pipe-lining ). This makes testing against null response scenarios in unified fashion a bit irrelevant.

Zooming out a bit, discerning between scenarios where response is expected and those where the situation is opposite would be IMPOSSIBLE with the current state of the codebase.

IMO, The correct way to proceed from here is to try to catch the original error redis sends back when it detects a bad op ( write to read-only slave) prior to calling the exec.

By not discarding JedisDataExecptions that are returned from getMany and verifying to make none-of them exists. What's the original reason for discarding these errors ??

[sheinbergon]

@marcosnils any response ???

[sheinbergon]

@marcosnils ???

[marcosnils]

@sheinbergon I've been super busy lately. I'll try to review this week. @HeartSaVioR can you help me out if you have some time?

[sheinbergon]

@marcosnils @HeartSaVioR any response ???

[nick-benoit14]

Any more word on this?

[github-actions[bot]]

This issue is marked stale. It will be closed in 30 days if it is not updated.