ReviewOS
Reload

also looking at this

craft-native/craft

refactor(ios): lift #211's owed-block design out of Siri, so other actions can use it

#245
Merged glennmichael123 wants to merge fix/ios-pending-call-table into main
2 files +248 -195
packages/zig/src/bridge_mobile_siri.zigmodified+32-195
@@ -58,6 +58,7 @@ const objc_runtime = ;
5858const ios_async = ;
5959const compat = ;
6060const compat_mutex = ;
61const ios_pending = ;
6162
6263const objc = objc_runtime.objc;
6364const BridgeError = bridge_error.BridgeError;
@@ -175,7 +176,7 @@ pub const SiriBridge = struct {
175176 };
176177 errdefer ios_async.abandon(ticket);
177178
178 const block = claimBlock(ticket, reply) orelse {
179 const block = calls.claim(ticket, .{ .reply = reply }) orelse {
179180 std.log.warn(
180181 "removeSiriShortcut: all {d} completion blocks are still owed a completion " ++
181182 "Foundation never delivered; refusing rather than reusing one",
@@ -392,91 +393,24 @@ const removal_deadline_ms: u32 = 15_000;
392393/// the reply needs is shaped before the call and parked here. The ticket is
393394/// stored rather than rebuilt, because a `Ticket` is an index *and* a
394395/// generation, and the deadline matches on both.
396/// What this action parks: the finished reply, shaped before the call because
397/// `void (^)(void)` carries nothing — not even a success flag.
395398const PendingCall = struct {
396 ticket: ios_async.Ticket,
397399 /// Owned, `c_allocator`. Freed by whichever of the completion and the
398400 /// deadline takes the call.
399401 reply: []u8,
400 started_ms: i64,
401402};
402403
403/// One per completion block, not per reply slot.
404/// One entry per completion block, not per reply slot.
404405///
405406/// A block knows only its own index. If blocks were chosen by reply slot, a
406407/// deadline would release slot k, the next removal would lease slot k and get
407408/// the same block, and the first removal's late completion would then answer
408/// the second with `removed: true` before its deletion had finished. So a
409/// block whose call the deadline answered stays `owed` until Foundation calls
410/// it, and is never handed to another call in the meantime.
411const BlockState = union(enum) {
412 free,
413 waiting: PendingCall,
414 /// Answered by the deadline. Foundation still holds this block and may
415 /// call it; when it does, that call is ignored and the block is freed.
416 /// Carries when the call started, so a late completion can say how late.
417 owed: i64,
418};
419
420/// Twice the reply slots, so every slot can be waiting while as many blocks
421/// again are still owed a completion that has not come.
422const block_count = 2 * ios_async.max_in_flight;
423
424var blocks: [block_count]BlockState = @splat(.free);
425var pending_mutex: compat_mutex.Mutex = .{};
426
427/// Park `reply` on the first free block. Null when every block is waiting or
428/// owed, which the caller refuses rather than reusing an owed block.
429fn claimBlock(ticket: ios_async.Ticket, reply: []u8) ?u5 {
430 pending_mutex.lock();
431 defer pending_mutex.unlock();
432 for (&blocks, 0..) |*state, i| {
433 if (state.* != .free) continue;
434 state.* = .{ .waiting = .{ .ticket = ticket, .reply = reply, .started_ms = compat.milliTimestamp() } };
435 return @intCast(i);
436 }
437 return null;
438}
439
440/// The deadline's half: take the call `ticket` names if it is still waiting,
441/// and leave its block owed. Matching the whole ticket is what stops a timer
442/// from expiring a newer call that leased the same reply slot.
443fn expireIfWaiting(ticket: ios_async.Ticket) ?PendingCall {
444 pending_mutex.lock();
445 defer pending_mutex.unlock();
446 for (&blocks) |*state| {
447 const call = switch (state.*) {
448 .waiting => |call| call,
449 else => continue,
450 };
451 if (call.ticket.index != ticket.index or call.ticket.generation != ticket.generation) continue;
452 state.* = .{ .owed = call.started_ms };
453 return call;
454 }
455 return null;
456}
457
458const Settled = union(enum) {
459 /// The completion arrived first: answer with this call.
460 reply: PendingCall,
461 /// The deadline already answered. Nothing to send. When the call started.
462 late: i64,
463 /// No call was ever parked on this block.
464 stray,
465};
466
467/// The completion's half. Each block leaves `waiting` or `owed` exactly once,
468/// under the lock, so the completion and the deadline cannot both answer.
469fn settleBlock(block: u5) Settled {
470 pending_mutex.lock();
471 defer pending_mutex.unlock();
472 const state = blocks[block];
473 blocks[block] = .free;
474 return switch (state) {
475 .waiting => |call| .{ .reply = call },
476 .owed => |started_ms| .{ .late = started_ms },
477 .free => .stray,
478 };
479}
409/// the second with `removed: true` before its deletion had finished. The table
410/// is what keeps a block owed until the framework calls it; `ios_pending`
411/// carries the whole design, and the other actions in #223 need the same.
412var calls: ios_pending.Table(PendingCall) = .{};
413const block_count = ios_pending.block_count;
480414
481415const BlockDescriptor = extern struct {
482416 reserved: c_ulong = 0,
@@ -531,14 +465,14 @@ var done_blocks: [block_count]DoneBlock =
531465fn deletionFired(block: u5) void {
532466 if (!is_darwin) return;
533467
534 switch (settleBlock(block)) {
535 .reply => |call| {
536 defer std.heap.c_allocator.free(call.reply);
468 switch (calls.settle(block)) {
469 .reply => |entry| {
470 defer std.heap.c_allocator.free(entry.call.reply);
537471 std.log.info(
538472 "removeSiriShortcut: the deletion completion arrived after {d} ms",
539 .{compat.milliTimestamp() - call.started_ms},
473 .{compat.milliTimestamp() - entry.started_ms},
540474 );
541 ios_async.deliverJson(call.ticket, call.reply);
475 ios_async.deliverJson(entry.ticket, entry.call.reply);
542476 },
543477 .late => |started_ms| std.log.warn(
544478 "removeSiriShortcut: a deletion completion arrived {d} ms after the call, after " ++
@@ -558,8 +492,8 @@ fn deletionFired(block: u5) void {
558492fn removalTimedOut(context: ?*anyopaque) callconv(.c) void {
559493 if (!is_darwin) return;
560494
561 const call = expireIfWaiting(ios_async.ticketFromDeadline(context)) orelse return;
562 defer std.heap.c_allocator.free(call.reply);
495 const entry = calls.expireIfWaiting(ios_async.ticketFromDeadline(context)) orelse return;
496 defer std.heap.c_allocator.free(entry.call.reply);
563497
564498 // The page's message is fixed by the error code, so the framework is named
565499 // here. No `i=` in this line: the slice fixture greps for markers by it.
@@ -568,7 +502,7 @@ fn removalTimedOut(context: ?*anyopaque) callconv(.c) void {
568502 "completionHandler:] did not call its completion within {d} ms; answering TIMEOUT",
569503 .{removal_deadline_ms},
570504 );
571 ios_async.deliverErrorCode(call.ticket, BridgeError.Timeout);
505 ios_async.deliverErrorCode(entry.ticket, BridgeError.Timeout);
572506}
573507
574508const testing = std.testing;
@@ -650,9 +584,8 @@ test "a refused removal leases no reply slot" {
650584 // No `action`, so it refuses before the ticket is taken.
651585 bridge.handleMessage(A.remove_siri_shortcut, "{}") catch {};
652586
653 pending_mutex.lock();
654 defer pending_mutex.unlock();
655 for (blocks) |state| try testing.expect(state == .free);
587 // Nothing parked: every block settles as a stray.
588 for (0..block_count) |i| try testing.expect(calls.settle(@intCast(i)) == .stray);
656589}
657590
658591test "each completion block is global and has its own invoke" {
@@ -685,14 +618,11 @@ test "a completion for a block with no recorded call is ignored" {
685618
686619/// Free every block, and any reply a previous test left parked.
687620fn resetBlocks() void {
688 pending_mutex.lock();
689 defer pending_mutex.unlock();
690 for (&blocks) |*state| {
691 switch (state.*) {
692 .waiting => |call| std.heap.c_allocator.free(call.reply),
621 for (0..block_count) |i| {
622 switch (calls.settle(@intCast(i))) {
623 .reply => |entry| std.heap.c_allocator.free(entry.call.reply),
693624 else => {},
694625 }
695 state.* = .free;
696626 }
697627}
698628
@@ -700,115 +630,22 @@ fn parkedReply(text: []const u8) ![]u8 {
700630 return std.heap.c_allocator.dupe(u8, text);
701631}
702632
703test "a deadline answers a waiting removal once, and leaves its block owed" {
704 resetBlocks();
705 defer resetBlocks();
706
707 const ticket: ios_async.Ticket = .{ .index = 3, .generation = 7 };
708 const block = claimBlock(ticket, try parkedReply("{\"removed\":true}")) orelse return error.NoBlock;
709
710 const call = expireIfWaiting(ticket) orelse return error.DeadlineFoundNothing;
711 defer std.heap.c_allocator.free(call.reply);
712 try testing.expectEqualStrings("{\"removed\":true}", call.reply);
713 try testing.expect(blocks[block] == .owed);
714
715 // A second expiry of the same call finds nothing to answer.
716 try testing.expect(expireIfWaiting(ticket) == null);
717}
718
719test "a completion after its deadline is ignored, and frees its block" {
720 resetBlocks();
721 defer resetBlocks();
722
723 const ticket: ios_async.Ticket = .{ .index = 3, .generation = 7 };
724 const block = claimBlock(ticket, try parkedReply("r")) orelse return error.NoBlock;
725 const call = expireIfWaiting(ticket) orelse return error.DeadlineFoundNothing;
726 std.heap.c_allocator.free(call.reply);
727
728 try testing.expect(settleBlock(block) == .late);
729 try testing.expect(blocks[block] == .free);
730 try testing.expect(settleBlock(block) == .stray);
731}
732
733test "a deadline that fires after the completion does nothing" {
734 // The usual case: the timer cannot be cancelled, so it fires for every
735 // removal, including the ones that were answered long ago.
736 resetBlocks();
737 defer resetBlocks();
738
739 const ticket: ios_async.Ticket = .{ .index = 3, .generation = 7 };
740 const block = claimBlock(ticket, try parkedReply("r")) orelse return error.NoBlock;
741 switch (settleBlock(block)) {
742 .reply => |call| std.heap.c_allocator.free(call.reply),
743 else => return error.CompletionFoundNothing,
744 }
745
746 try testing.expect(expireIfWaiting(ticket) == null);
747 try testing.expect(blocks[block] == .free);
748}
749
750test "a stale deadline cannot expire a newer removal on the same reply slot" {
751 resetBlocks();
752 defer resetBlocks();
753
754 const first: ios_async.Ticket = .{ .index = 3, .generation = 7 };
755 const block = claimBlock(first, try parkedReply("first")) orelse return error.NoBlock;
756 switch (settleBlock(block)) {
757 .reply => |call| std.heap.c_allocator.free(call.reply),
758 else => return error.CompletionFoundNothing,
759 }
760
761 const second: ios_async.Ticket = .{ .index = 3, .generation = 9 };
762 const again = claimBlock(second, try parkedReply("second")) orelse return error.NoBlock;
763
764 try testing.expect(expireIfWaiting(first) == null);
765 switch (blocks[again]) {
766 .waiting => |call| try testing.expectEqual(@as(u32, 9), call.ticket.generation),
767 else => return error.NewerCallWasExpired,
768 }
769}
770
771test "a reply slot reused after a deadline never gets the block still owed" {
772 // The mix-up a deadline keyed only by reply slot would ship: the first
773 // removal's late completion answering the second with `removed: true`
774 // before the second deletion had finished.
775 resetBlocks();
776 defer resetBlocks();
777
778 const first: ios_async.Ticket = .{ .index = 0, .generation = 1 };
779 const first_block = claimBlock(first, try parkedReply("first")) orelse return error.NoBlock;
780 const expired = expireIfWaiting(first) orelse return error.DeadlineFoundNothing;
781 std.heap.c_allocator.free(expired.reply);
782
783 // The same slot index, as `ios_async.acquire` hands out the lowest free one.
784 const second: ios_async.Ticket = .{ .index = 0, .generation = 3 };
785 const second_block = claimBlock(second, try parkedReply("second")) orelse return error.NoBlock;
786 try testing.expect(second_block != first_block);
787
788 // The late completion for the first removal answers nobody.
789 try testing.expect(settleBlock(first_block) == .late);
790 switch (settleBlock(second_block)) {
791 .reply => |call| {
792 defer std.heap.c_allocator.free(call.reply);
793 try testing.expectEqual(@as(u32, 3), call.ticket.generation);
794 try testing.expectEqualStrings("second", call.reply);
795 },
796 else => return error.SecondCallLost,
797 }
798}
799
800633test "with every block still owed, a removal is refused rather than reusing one" {
634 // Siri's half of the refusal: the generic table returning null is covered
635 // in ios_pending; this is that this action declines instead of reusing.
801636 resetBlocks();
802637 defer resetBlocks();
803638
804 pending_mutex.lock();
805 for (&blocks) |*state| state.* = .{ .owed = 0 };
806 pending_mutex.unlock();
639 for (0..block_count) |i| {
640 const ticket: ios_async.Ticket = .{ .index = @intCast(i % ios_async.max_in_flight), .generation = @intCast(i + 1) };
641 _ = calls.claim(ticket, .{ .reply = try parkedReply("r") }) orelse return error.NoBlock;
642 const owed = calls.expireIfWaiting(ticket) orelse return error.DeadlineFoundNothing;
643 std.heap.c_allocator.free(owed.call.reply);
644 }
807645
808646 const reply = try parkedReply("r");
809647 defer std.heap.c_allocator.free(reply);
810 try testing.expect(claimBlock(.{ .index = 0, .generation = 1 }, reply) == null);
811 for (blocks) |state| try testing.expect(state == .owed);
648 try testing.expect(calls.claim(.{ .index = 0, .generation = 999 }, .{ .reply = reply }) == null);
812649}
813650
814651test "the deadline answers before the page's own request timeout" {