brson · GitHub

This fixes a data race-induced crash in tb_client upon eviction. The race always occurs if an eviction is followed by a submit and cannot be worked around, though it doesn't always crash.

Problem

(This is copied from a quick writeup I did earlier).

Here's my current understanding of the client eviction crash.
This is not the windows crash but may be related.

The three functions most in play here are client_eviction_callback,
vtable_submit_fn, and io_thread.
client_eviction_callback and io_thread run on the io_thread,
vtable_submit_fn runs on any thread.

The semantics that i think are expressed in the code are:
client_eviction_callback shuts down the io thread while
letting client-handle holders / transaction submitters know
"we're evicted. stop submitting", with the expectation
that at this point the client is shut down.
the fundamental issue this needs to solve is that
other threads may be submitting transactions at the same time.

here's the guy that starts the bug:

        fn client_eviction_callback(client: *Client, eviction: *const Message.Eviction) void {
            // ... snip ...
            self.eviction_reason = eviction.header.reason;
            self.signal.stop();
        }

This function on the io thread is not called under lock and eviction_reason is shared but not atomic.
that's wrong, but it's not the main problem.

Hitting the stop signal wakes up the io loop (which is on the same thread in this case)
and causes it to start its shutdown sequence.
At this point there may be transactions at various stages of submission that can't be stopped - somehow they either need to go all the way to the server, or to be short-circuited and in either case a response given to the client.

now submit, which runs on any thread:

        fn vtable_submit_fn(context: *anyopaque, packet_extern: *Packet.Extern) void {
            // ... snip ...
            if (self.eviction_reason == null) {
                // Enqueue the packet and notify the IO thread to process it asynchronously.
                assert(self.signal.status() == .running);
                self.submitted.push(packet);
                self.signal.notify();
            } else {
                // Cancel the packet since we stop the IO thread during eviction.
                assert(self.signal.status() != .running);
                self.packet_cancel(packet);
            }
        }

This function is called under lock but it doesn't protect eviction_reason because
the other thread doesn't take the lock.
Even if it was under lock that's still not the problem.

Note that thread sanitizer doesn't see the eviction_reason data race, which looks clear to me;
I don't hthink that Zig 0.14's thread sanitizer instrumentation is complete.

This if/else though should raise alarms because the else is a corner
case that has oddly different synchronization requirements
than any other code in the client: normally the io thread
is responsible for calling packet_cancel, but in this case
suddenly packet_cancel is being called on an arbitrary thread.

That's also not the problem.

This entire if-else block interacts catastrophically with the io_thread,
which is already in the process of shutting itself down and deleting some
of the values being called here in both branches (because eviction_callback
called signal.stop).

        fn io_thread(self: *Context) void {
            while (self.signal.status() != .stopped) {
                self.tick();
                self.io.run_for_ns(constants.tick_ms * std.time.ns_per_ms) catch |err| {
                    log.err("{}: IO.run() failed: {s}", .{
                        self.client_id,
                        @errorName(err),
                    });
                    @panic("IO.run() failed");
                };
            }
            self.cancel_request_inflight();
            while (self.pending.pop()) |packet| {
                packet.assert_phase(.pending);
                self.packet_cancel(packet);
            }
            // The submitted queue is no longer accessible to user threads,
            // so synchronization is not required here.
            while (self.submitted.pop()) |packet| {
                packet.assert_phase(.submitted);
                self.packet_cancel(packet);
            }
            self.io.cancel_all();
            self.signal.deinit();
            self.client.deinit(self.gpa.allocator());
            self.message_pool.deinit(self.gpa.allocator());
            self.io.deinit();
        }

Once stop is called the io thread loop exits and all that shutdown code
runs, unsynchronized with client threads, one of which is currently
calling submit.

The manifestation of this is that one or any of the signal methods gets called by submit in a client thread on a destroyed signal value.

I don't have the actual crash output but it usually happens
in this function, called from submit:

    pub fn notify(self: *Signal) void {
        // Try to transition from `waiting` to `notified`.
        // If it fails, analyze the current state to determine if a notification is needed.
        var state: @TypeOf(self.event_state.raw) = .waiting;
        while (self.event_state.cmpxchgStrong(
            state,
            .notified,
            .release,
            .acquire,
        )) |state_actual| {
            switch (state_actual) { // XXXXX crash "switch on corrupt value"
                .waiting, .running => state = state_actual, // Try again.
                .notified => return, // Already notified.
                .shutdown => return, // Ignore notifications after shutdown.
            }
        }
        // snip
    }

The fix

The main insight into the fix is that the client context should not attempt to shut down the io thread itself - this creates a "bimodal" situation where a rare corner case has very different parallel/synchronization behavior, with client submission threads being required to handle packet cancellation, and is hard to reason about and test. After the patch the io thread runs after eviction and continues short-circuiting packet_cancel until the client calls the destructor.

NOTE that the semantics of eviction are that after a client is evicted all subsequent requests report evicted, and the client must notice this and tear down the client and start over.
This was true before but was possibly not well understood, and the docs I think are misleading and need to be updated prominently.

This is the current doc on the subject:

The cluster sends a message to notify the evicted session that it has ended. Typically the evicted
client is no longer active (already terminated), but if it is active, the eviction message causes it
to self-terminate, bubbling up to the application as an session evicted error.

The 'self-terminate' phrase is misleading - the client continues to exist and can be called even after an eviction, and the only solution is to destroy and recreate. This also means that callers that are sharing the client will need to coordinate its destruction while handling continued eviction responses.

It is not easy to produce a test that quickly and reliably causes the crash using only the public API, so I had to add a private testing API to trigger the eviction.

Read the original on github.com ↗