ReviewOS

also looking at this

craft-native/craft

fix(ios): resolve openSettings' selector before the lease, and say why two actions have no deadline

#251
Merged glennmichael123 wants to merge fix/ios-open-settings-lease into main
2 files +25 -2
9 days ago

Refs #223 — its last two actions, and a proposal to close it.

One bug, fixed

openSettings looked up openURL:options:completionHandler: after publishSettingsCall. A failure there released the reply slot through the errdefer, but left the ticket parked in settings_calls for a call that had already been refused. It now resolves before acquire, which is the rule #223 asks of every action and the one the other four already follow.

Two actions that should not get a deadline

#223 lists openURL and openSettings alongside the four that now have one. Having read both, I think neither should, and each now says why at the call — so nobody later adds one believing it is a fix.

openSettings: a deadline would report a success as a timeout

The app is suspended while Settings is open. The deadline's clock is uptime, which pauses only while the device sleeps, not while the app is suspended — ios_async's own comment on scheduleDeadline records this.

So a person who spends longer in Settings than the deadline returns to the app with both the expired deadline and the held-back completion pending on the main queue. They race. If the deadline runs first, the page hears TIMEOUT for an open that worked.

That is not an edge case. People open Settings to change something, and a visit longer than any sensible deadline is the ordinary case. #223 anticipated exactly this in its own table.

What a deadline would buy is protection against openURL:options:completionHandler: never calling back. UIKit calls it on every open. The side table is also safe without one: #223's concern is a deadline freeing a slot early, and nothing frees this slot early.

openURL: #223's reason for wanting a deadline does not apply

This one is structurally different from the other five. It leases no ios_async reply slot at all. The request id travels inside the block, which openURL:options:completionHandler: heap-copies, and the completion replies with it directly.

#223's core concern is a deadline freeing reply slot k and handing its block to the next call. There is no slot here to free.

What a deadline would do is time out a person: tel: and facetime: put a "Call …?" confirmation on screen, and the completion waits for the answer. #223 itself called this one borderline for that reason.

Proposal: close #223 with this merged

actionoutcome
getPendingNotificationsdeadline — #247
scheduleNotificationdeadline, armed after the grant — #248
getHealthDatadeadline — #250
saveHealthWorkoutdeadline per stage — #250
openSettingsno deadline, reason at the call; lease ordering fixed here
openURLno deadline, reason at the call

Plus #245's shared table, #249's per-claim ratchet, and every fallible step #223 listed now resolving before its lease.

Verification

zig build test:ios, Pickier (0 errors). Comments and one reordering; no behaviour changes on the success path.

2 changed files on the files tab, with 0 review threads.

Sign in to comment on this pull request.