search

aembke / fred.rs

An async Redis client for Rust.
Apache License 2.0
367 stars 58 forks source link

[Bug] Transactions with WATCH/UNWATCH do not work #251

Closed sitano closed 3 weeks ago

sitano commented 3 months ago

Fred version - 9.0.3 Redis version - 6.2.13 Platform - linux+mac (docker on macos) Deployment type - cluster (3 nodes)

Describe the bug

Transactions (https://redis.io/docs/latest/develop/interact/transactions/#watch-explained) do not work correctly:

WATCH a
a = GET a
if a > 1:
  MULTI
    a = a+1
    SET a
  EXEC
  1. It is possible that any concurrent client will corrupt the Connection state in RedisClient by using a RedisPool.
1: WATCH a
1: val = GET a ; a == 10
2: UNWATCH a
2: SET a 0 ; write 0 to a
1: if val > 1:
1:   MULTI
1:     val = val+1
1:     SET a val
1:   EXEC ; will do well because a is unwatched
---
a == 11
  1. UNWATCH is sent to the random node in RedisClient.
  2. it looks to me that .watch_before() is meaningless. if you do not do anything in between WATCH + MULTI what sense does it make?

To Reproduce Steps to reproduce the behavior:

imagine there are 2 concurrent threads running client1 and client2. run test.rs.txt multiple times:

$ RUST_LOG=debug cargo run

Logs (If possible set RUST_LOG=fred=trace and run with --features debug-ids)

possible outcomes - UNWATCH fire random node, TX execute on written value:

2024-06-10 14:37:29,648 DEBUG [fred::router::clustered] fred-HsL4F8SjOf: Writing command `WATCH` to 127.0.0.1:20252
2024-06-10 14:37:29,648 DEBUG [fred::router::clustered] fred-HsL4F8SjOf: Writing command `GET` to 127.0.0.1:20252
2024-06-10 14:37:29,649 DEBUG [fred::router::clustered] fred-HsL4F8SjOf: Writing command `UNWATCH` to 127.0.0.1:20251
2024-06-10 14:37:29,649 DEBUG [fred::router::clustered] fred-HsL4F8SjOf: Writing command `SET` to 127.0.0.1:20252
2024-06-10 14:37:29,650 DEBUG [fred::router::clustered] fred-HsL4F8SjOf: Writing command `UNWATCH` to 127.0.0.1:20252
2024-06-10 14:37:29,650 DEBUG [fred::router::transactions] fred-HsL4F8SjOf: Starting transaction 16753794841238535464 (attempted: 0)
2024-06-10 14:37:29,650 DEBUG [fred::router] fred-HsL4F8SjOf: Direct write `MULTI` command to 127.0.0.1:20252, ID: 0
2024-06-10 14:37:29,651 DEBUG [fred::router] fred-HsL4F8SjOf: Direct write `SET` command to 127.0.0.1:20252, ID: 0
2024-06-10 14:37:29,651 DEBUG [fred::router] fred-HsL4F8SjOf: Direct write `EXEC` command to 127.0.0.1:20252, ID: 0
2024-06-10 14:37:29,651 DEBUG [fred::router::clustered] fred-HsL4F8SjOf: Writing command `GET` to 127.0.0.1:20252
done: Some(15) -> 25:  Some(5)

or UNWATCH missed the node

2024-06-10 14:42:08,574 DEBUG [fred::router::clustered] fred-kN6OTgoXgq: Writing command `WATCH` to 127.0.0.1:20252
2024-06-10 14:42:08,575 DEBUG [fred::router::clustered] fred-kN6OTgoXgq: Writing command `GET` to 127.0.0.1:20252
2024-06-10 14:42:08,575 DEBUG [fred::router::clustered] fred-kN6OTgoXgq: Writing command `UNWATCH` to 127.0.0.1:20251
2024-06-10 14:42:08,576 DEBUG [fred::router::clustered] fred-kN6OTgoXgq: Writing command `SET` to 127.0.0.1:20252
2024-06-10 14:42:08,576 DEBUG [fred::router::clustered] fred-kN6OTgoXgq: Writing command `UNWATCH` to 127.0.0.1:20250
2024-06-10 14:42:08,577 DEBUG [fred::router::transactions] fred-kN6OTgoXgq: Starting transaction 9983206465424982182 (attempted: 0)
2024-06-10 14:42:08,577 DEBUG [fred::router] fred-kN6OTgoXgq: Direct write `MULTI` command to 127.0.0.1:20252, ID: 0
2024-06-10 14:42:08,577 DEBUG [fred::router] fred-kN6OTgoXgq: Direct write `SET` command to 127.0.0.1:20252, ID: 0
2024-06-10 14:42:08,578 DEBUG [fred::router] fred-kN6OTgoXgq: Direct write `EXEC` command to 127.0.0.1:20252, ID: 0
2024-06-10 14:42:08,579 DEBUG [fred::router::clustered] fred-kN6OTgoXgq: Writing command `GET` to 127.0.0.1:20252
not done: Some(35) -> 5:  None
aembke commented 2 months ago

Hi @sitano. Good point - the shared nature of the current RedisPool interface prevents this kind of use case from working correctly.

9.1.0 (https://github.com/aembke/fred.rs/pull/248) adds an ExclusivePool interface that should work here. I added a test that shows how this might work: https://github.com/aembke/fred.rs/blob/feat/9.1.0/tests/integration/pool/mod.rs#L76-L110

aembke commented 3 weeks ago

This should be working via the new pool interface in https://github.com/aembke/fred.rs/releases/tag/v9.1.0, but let me know if you still have any issues.