java: add a test for concurrent close

[matklad]

No description provided.

[matklad]

This currently crashes the JVM with

thread 1063421 panic: reached unreachable code
/Users/matklad/p/tb/work/zig/lib/std/debug.zig:412:14: 0x10ed3cf1f in assert (tb_jniclient)
    if (!ok) unreachable; // assertion failure
             ^
/Users/matklad/p/tb/work/src/clients/c/tb_client/context.zig:529:23: 0x10edc5dc3 in cancel (tb_jniclient)
                assert(batched.batch_next == null or batched.batch_allowed);
                      ^
/Users/matklad/p/tb/work/src/clients/c/tb_client/context.zig:282:62: 0x10eddc387 in run (tb_jniclient)
            while (self.submitted.pop()) |packet| self.cancel(packet);
                                                             ^
/Users/matklad/p/tb/work/zig/lib/std/Thread.zig:408:13: 0x10edc7913 in callFn__anon_11237 (tb_jniclient)
            @call(.auto, f, args);
            ^
/Users/matklad/p/tb/work/zig/lib/std/Thread.zig:674:30: 0x10ed9867f in entryFn (tb_jniclient)
                return callFn(f, args_ptr.*);
                             ^
???:?:?: 0x19b48b2e3 in ??? (libsystem_pthread.dylib)
???:?:?: 0x19b4860fb in ??? (libsystem_pthread.dylib)
info(main): stdin closed, exiting

[matklad]

cc @kprotty, @batiati could you take a look at what's going on here?

[matklad]

Debugged:

• tigerbeetle/src/clients/java/src/client.zig

  Lines 127 to 133 in 0b764aa

  	packet.operation = operation;
  	packet.user_data = global_ref;
  	packet.data = send_buffer.ptr;
  	packet.data_size = @intCast(send_buffer.len);
  	packet.next = null;
  	packet.status = .ok;

  -- here we don't initialized a bunch of fields, like batch_next and such

• tigerbeetle/src/clients/c/tb_client/context.zig

  Lines 421 to 425 in e6ede94

  	// On shutdown, cancel this packet as well as any others batched onto it.
  	if (self.shutdown.load(.acquire)) {
  	self.cancel(packet);
  	return;
  	}

  -- here we send this partially-initialized packet to the cancelation routine

• tigerbeetle/src/clients/c/tb_client/context.zig

  Line 530 in e6ede94

  it = batched.batch_next;

  -- here we dereferencing wild pointers!

[kprotty]

packets come in through self.submitted and either go to request or cancel directly through the while loop on shutdown. The request path is normally the one to initialize the packet.batch* fields before potentially calling cancel but the while-loop path doesnt.

I think the while-loop path should go through request which set the right fields, sees shutdown before reaching vsr.Client, and eventually cancels it.

[batiati]

We could simplify this API by moving these fields to a separate struct (opaque to the client libraries).

Example:

// Exposed to the client.
pub const PacketData = extern struct {
    user_data: ?*anyopaque,
    operation: u8,
    status: Packet.Status,
    data_size: u32,
    data: ?*anyopaque,
    reserved: [42]u8 = [_]u8{0} ** 42, // Internal fields are just opaque.

    comptime {
        assert(@sizeOf(PacketData) == @sizeOf(Packet));
        assert(@alignOf(PacketData) == @alignOf(Packet));
    }
};

// Internal implementation.
pub const Packet = extern struct {
    user_data: ?*anyopaque,
    operation: u8,
    status: Status,
    data_size: u32,
    data: ?*anyopaque,
    next: ?*Packet,
    batch_next: ?*Packet,
    batch_tail: ?*Packet,
    batch_size: u32,
    batch_allowed: bool,
    reserved: [7]u8 = [_]u8{0} ** 7,

    comptime {
        assert(@sizeOf(Packet) == 64);
        assert(@alignOf(Packet) == 8);
    }
}

[matklad]

Ok, so I tried follow batiati's suggestion to refactor the API, but then figured out that that's going to be a large change, and that I don't want to mix bug-fix and a refactor in the same PR. I still want to do a refactor, but I want to more carefully survey our entire C ABI layer before that :P

So I decided to go with kprotty's suggestion instead, and to make sure that the cancellation path also goes through .request. This indeed feels like the right thing to do as, it reduces dimensionality (a packet always goes through on_submit -> request -> cancel OR complete). But that, sadly, didn't work --- request has "registration was completed" as a pre-condition, but, I think, we could get canceled before registration finishes. I added an assert to that end.

So I think I still want to merge what we have here now, to fix the bug in the most straightforward way, and then revisit the wider context later!

[batiati]

Note when refactoring tb_packet_t:

status is the only field that is both exposed to the client and updated by tb_client. We should change this API so that the status is instead returned alongside results.