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" {
packages/zig/src/ios_pending.zigadded+216-0
@@ -0,0 +1,216 @@
1//! A call parked on a completion block, answerable from either side exactly once.
2//!
3//! `ios_async` hands out reply slots, and a slot is released the moment
4//! something answers it. That is right for the reply, and wrong for the block:
5//! a deadline releases slot k, the next call leases slot k, and — if blocks
6//! were chosen by reply slot — the first call's late completion would fire the
7//! same block and answer the second call with the first one's result.
8//!
9//! #211 found that in `removeSiriShortcut`, where the only answer came from a
10//! system daemon that sometimes never called back. The fix there was to make
11//! blocks their own resource: a block whose call the deadline answered stays
12//! `owed` until the framework calls it, and is never handed to another call in
13//! the meantime. This file is that design, lifted out so the other actions with
14//! the same shape can take a deadline safely (#223).
15//!
16//! What a caller still owns: the block array itself (its shape is the
17//! framework's, `void (^)(void)` for Siri and `void (^)(NSArray *)` elsewhere),
18//! and what it parks. This owns only who may answer, and when.
19
20const std = ;
21const ios_async = ;
22const compat = ;
23const compat_mutex = ;
24
25/// Twice the reply slots, so every slot can be waiting while as many blocks
26/// again are still owed a completion that has not come.
27pub const block_count = 2 * ios_async.max_in_flight;
28
29/// `Call` is whatever the module needs to answer: a shaped reply, a selector
30/// pair, a chain of HealthKit stages. This file neither reads nor frees it —
31/// whichever side takes the entry owns it from then on.
32pub fn Table(comptime Call: type) type {
33 return struct {
34 const Self = @This();
35
36 /// A parked call. The ticket is stored rather than rebuilt, because a
37 /// `Ticket` is an index *and* a generation and the deadline matches on
38 /// both — matching on the index alone is exactly the bug this prevents.
39 pub const Entry = struct {
40 ticket: ios_async.Ticket,
41 call: Call,
42 started_ms: i64,
43 };
44
45 const State = union(enum) {
46 free,
47 waiting: Entry,
48 /// Answered by the deadline. The framework still holds this block
49 /// and may call it; when it does, that call is ignored and the
50 /// block is freed. Carries when the call started, so a late
51 /// completion can say how late it was.
52 owed: i64,
53 };
54
55 pub const Settled = union(enum) {
56 /// The completion arrived first: answer with this call.
57 reply: Entry,
58 /// The deadline already answered. Nothing to send, and when it began.
59 late: i64,
60 /// No call was ever parked on this block.
61 stray,
62 };
63
64 blocks: [block_count]State = @splat(.free),
65 mutex: compat_mutex.Mutex = .{},
66
67 /// Park `call` on the first free block.
68 ///
69 /// Null when every block is waiting or owed. The caller refuses the
70 /// action rather than reusing an owed block — a refusal the page can
71 /// act on, against an answer that would go to the wrong call.
72 pub fn claim(self: *Self, ticket: ios_async.Ticket, call: Call) ?u5 {
73 self.mutex.lock();
74 defer self.mutex.unlock();
75 for (&self.blocks, 0..) |*state, i| {
76 if (state.* != .free) continue;
77 state.* = .{ .waiting = .{
78 .ticket = ticket,
79 .call = call,
80 .started_ms = compat.milliTimestamp(),
81 } };
82 return @intCast(i);
83 }
84 return null;
85 }
86
87 /// The deadline's half: take the call `ticket` names if it is still
88 /// waiting, and leave its block owed.
89 ///
90 /// Matching the whole ticket is what stops a timer armed for one call
91 /// from expiring a newer call that leased the same reply slot.
92 pub fn expireIfWaiting(self: *Self, ticket: ios_async.Ticket) ?Entry {
93 self.mutex.lock();
94 defer self.mutex.unlock();
95 for (&self.blocks) |*state| {
96 const entry = switch (state.*) {
97 .waiting => |waiting| waiting,
98 else => continue,
99 };
100 if (entry.ticket.index != ticket.index or
101 entry.ticket.generation != ticket.generation) continue;
102 state.* = .{ .owed = entry.started_ms };
103 return entry;
104 }
105 return null;
106 }
107
108 /// The completion's half.
109 ///
110 /// Each block leaves `waiting` or `owed` exactly once, under the lock,
111 /// so the completion and the deadline cannot both answer.
112 pub fn settle(self: *Self, block: u5) Settled {
113 self.mutex.lock();
114 defer self.mutex.unlock();
115 const state = self.blocks[block];
116 self.blocks[block] = .free;
117 return switch (state) {
118 .waiting => |entry| .{ .reply = entry },
119 .owed => |started_ms| .{ .late = started_ms },
120 .free => .stray,
121 };
122 }
123
124 /// Test-only: drop every parked call without answering any of them.
125 pub fn resetForTest(self: *Self) void {
126 self.mutex.lock();
127 defer self.mutex.unlock();
128 self.blocks = @splat(.free);
129 }
130 };
131}
132
133const testing = std.testing;
134
135/// A payload with nothing in it: these tests are about who may answer.
136const Marker = struct { id: u32 };
137var table: Table(Marker) = .{};
138
139fn fresh() *Table(Marker) {
140 table.resetForTest();
141 return &table;
142}
143
144test "a deadline answers a waiting call once and leaves its block owed" {
145 const t = fresh();
146 const ticket: ios_async.Ticket = .{ .index = 3, .generation = 9 };
147 const block = t.claim(ticket, .{ .id = 1 }) orelse return error.NoFreeBlock;
148
149 const taken = t.expireIfWaiting(ticket) orelse return error.NothingToExpire;
150 try testing.expectEqual(@as(u32, 1), taken.call.id);
151 // Gone from the deadline's point of view, and not free for another call.
152 try testing.expect(t.expireIfWaiting(ticket) == null);
153 try testing.expect(t.claim(.{ .index = 4, .generation = 1 }, .{ .id = 2 }) != block);
154}
155
156test "a completion after its deadline is ignored, and frees the block" {
157 const t = fresh();
158 const ticket: ios_async.Ticket = .{ .index = 3, .generation = 9 };
159 const block = t.claim(ticket, .{ .id = 1 }) orelse return error.NoFreeBlock;
160 _ = t.expireIfWaiting(ticket);
161
162 switch (t.settle(block)) {
163 .late => {},
164 else => return error.LateCompletionAnswered,
165 }
166 // And now the block is reusable, which an owed one was not.
167 try testing.expectEqual(block, t.claim(.{ .index = 5, .generation = 2 }, .{ .id = 3 }).?);
168}
169
170test "a deadline that fires after the completion does nothing" {
171 const t = fresh();
172 const ticket: ios_async.Ticket = .{ .index = 3, .generation = 9 };
173 const block = t.claim(ticket, .{ .id = 1 }) orelse return error.NoFreeBlock;
174
175 switch (t.settle(block)) {
176 .reply => |entry| try testing.expectEqual(@as(u32, 1), entry.call.id),
177 else => return error.CompletionDidNotAnswer,
178 }
179 try testing.expect(t.expireIfWaiting(ticket) == null);
180}
181
182test "a stale deadline cannot expire a newer call on the same reply slot" {
183 // The whole reason blocks are not chosen by reply slot: slot 3 is answered
184 // and released, the next call leases slot 3, and the first call's timer
185 // still fires.
186 const t = fresh();
187 const stale: ios_async.Ticket = .{ .index = 3, .generation = 9 };
188 const current: ios_async.Ticket = .{ .index = 3, .generation = 10 };
189 _ = t.claim(current, .{ .id = 7 }) orelse return error.NoFreeBlock;
190
191 try testing.expect(t.expireIfWaiting(stale) == null);
192 const taken = t.expireIfWaiting(current) orelse return error.NothingToExpire;
193 try testing.expectEqual(@as(u32, 7), taken.call.id);
194}
195
196test "a settle on a block nobody parked on is a stray, not an answer" {
197 const t = fresh();
198 switch (t.settle(0)) {
199 .stray => {},
200 else => return error.StrayAnswered,
201 }
202}
203
204test "with every block owed, a call is refused rather than given one" {
205 const t = fresh();
206 for (0..block_count) |i| {
207 const ticket: ios_async.Ticket = .{ .index = @intCast(i % ios_async.max_in_flight), .generation = @intCast(i + 1) };
208 _ = t.claim(ticket, .{ .id = @intCast(i) }) orelse return error.NoFreeBlock;
209 _ = t.expireIfWaiting(ticket);
210 }
211 try testing.expect(t.claim(.{ .index = 0, .generation = 999 }, .{ .id = 0 }) == null);
212}
213
214test "there are more blocks than reply slots, so a full pool can still be owed" {
215 try testing.expect(block_count > ios_async.max_in_flight);
216}