Races on the reconnect thread and callbacks running during ~RedisAdapter can abort the process (std::terminate) #118

RedisAdapter version or revision

main at ff2d5b8; unchanged through the #108 → #109 → #111 stack (0c3b0e6).

Redis version and deployment mode

Redis 7.4.11 standalone (TCP).

Observed behavior

There are two related lifecycle races. Both end in std::terminate ("terminate called without an active exception").

1. Concurrent reconnect() callers race on _reconnect_thd.

_connecting.exchange(true) admits one caller, but the reconnect thread clears _connecting itself as its last statement. Suppose the admitted caller is descheduled after the std::thread is constructed but before it is move-assigned into _reconnect_thd, and the new thread finishes in that window. A second caller then passes the exchange and calls joinable()/join()/operator= on the same std::thread object concurrently. That is a data race, and move-assigning into a still-joinable std::thread calls std::terminate.

Any multi-threaded consumer whose calls hit reconnect(0) during an outage or restart is exposed: connected(), failed writes and failed reads all qualify.

• Stress: N threads call connected() every 200 µs while CLIENT KILL runs every 3 ms, for 30 s. main aborted in 3/3 runs at 1 CPU with 64 threads and 2/3 at 2 CPUs with 32 threads. The stack head behaves the same.
• gdb: std::__terminate ← std::thread::operator= ← RedisAdapter::reconnect ← RedisAdapter::connected.
• TSan reports the race on the std::thread object (joinable() against operator= in another caller).

2. Callbacks that run during ~RedisAdapter can start a reconnect thread or restart readers after the destructor joined them.

_replier_pool is the last member, so worker callbacks keep running during the destructor body and are joined only afterwards. reconnect() reads _shutdown without synchronisation.

• A callback that makes a failing adapter call can pass that check just before the destructor sets it. It then joins or assigns _reconnect_thd concurrently with, or after, the destructor's own join. On main this was also seen as std::system_error (Invalid argument / No such process) thrown from the destructor's join.
• A late reconnect thread restarts readers the body had just stopped. The process then aborts when a joinable std::thread is destroyed.
• A callback that calls legacy removeReader() (bucket not empty), removeGenericReader() or setDeferReaders(false) restarts a reader thread the same way, because start_reader() has no _shutdown check. Add owned stream subscriptions and safe payload decoding #108 added one only to register_reader().

Measured with a callback that makes back-to-back failing calls (legacy reads of a wrong-type key, which reconnect; see #123) while the owner deletes the adapter:

TSan reports a race between ~RedisAdapter's _reconnect_thd.joinable() and _reconnect_thd = thread(...) in a worker, reached from getSingleValue.

Expected: reconnect() may be called from any thread, and callbacks may call adapter methods, without the process aborting.

Reproduction

• For (1): a stress harness, sketched in the first bullet above, using only APIs present on main.
• For (2): create an adapter with one reader. In its callback, loop getSingleValue on a key that holds a string (or call removeReader("other") while another key keeps the bucket non-empty). Delete the adapter 0-30 ms later from another thread. Build with -rdynamic for readable backtraces.

The harnesses are available on request.

Build and runtime environment

• Ubuntu 24.04, gcc 13.3 (plain and TSan)
• pinned redis-plus-plus 1.3.15 and hiredis 1.3.0
• a private Redis 7.4.11 in a container

Suggested fix

Validated: 0 aborts in 6 stress runs for (1); 0/129 plus 0/120 in an interleaved A/B for (2); 0/78 for the remove variant; lifecycle, recovery and gtest (14) pass.

• For (1), serialise every access to the thread handle, including the destructor's join. The reconnect thread never calls reconnect() itself, and no caller holds _reader_mtx while calling reconnect(0), so this cannot deadlock:

  if (result == 0 && !_connecting.exchange(true)) {
    std::lock_guard<std::mutex> lk(_reconnect_mtx);
    if (_reconnect_thd.joinable()) _reconnect_thd.join();
    _reconnect_thd = thread(...);
  }

• For (2), drain callbacks before joining anything. In ~RedisAdapter, right after _shutdown = true and clearing _reader_owner->adapter, call _replier_pool.shutdown(), a public wrapper for ThreadPool::stop(). A worker that calls it detaches itself, as stop() already does. Then join the watchdog and reconnect threads and stop the readers. Also add if (_shutdown) return false; at the top of start_reader().

Related:

• Detached reconnect thread can cause use-after-free #78: the member-thread pattern came from its fix in 95 data mover keeps crashing because of something in redis adapter #96.
• Data mover keeps crashing because of something in redis adapter. #95 and tcache alignment error #73: a plausible source of reconnect-time crashes, although the signature here is an abort rather than a tcache error.
• Add owned stream subscriptions and safe payload decoding #108: the _shutdown fence in register_reader.