Petrshtuka/thread safety - #24
Merged
Merged
Conversation
added 6 commits
August 18, 2026 15:49
`currentCallUUID`, `currentCaller`, `registrationError`, `configuration`, `provider`, `defaultCallerName`, `pushPayloadParser` and the coordinator's `currentAudioRoute` are object-typed properties written on one thread and read from another: a PJSIP callback thread resolving a caller name, the SIP queue reading the configuration, or whatever thread the host called a public method on. Unsynchronized that is an over-release, not a stale read, and the new regression test kills the process in under a second when the lock is removed from a single accessor. Every published property now goes through a lock-protected accessor. The declared surface stays `nonatomic` — the lock lives in a hand-written accessor rather than in the property attribute — so nothing changes for callers. The call projection (`callState`, `currentCallUUID`, `currentCaller`, `microphoneMuted`) additionally moves to the main queue for good: -stop and -providerDidReset: were the last two writers arriving from another thread, and they now hop through -clearPublishedCallState so an observer cannot catch a teardown half-applied. `isRunning`, `registrationState`, `registrationError` and `configuration` stay synchronous on purpose — start/login/stop return to a caller that reads them back immediately — and are safe because of the lock. The path monitor moves entirely onto the SIP queue, which is the queue it already delivers on, so its handle and the `hasObservedPath` bookkeeping have one owner instead of racing -stop. Verified with 128 tests under `-enableThreadSanitizer YES`: zero reports. The threading section of CallWaveKit/README.md now describes the contract the code actually keeps rather than the one it aimed at.
The last recorded device pass is 0.4.0 from 2026-08-04. Since then 0.5.0 shipped Opus, SHA-256 digest, QoS tagging and quality warnings, the audio session moved into CallWaveAudioSessionCoordinator, session timers arrived and the published state moved behind a lock — all in the code the unit suite explicitly does not reach. The record is committed empty and marked "not run", with a table of what changed and which scenarios each change puts at risk, so the pass starts from the highest-risk end rather than at scenario 1. It is deliberately not filled in: nothing here can be established from CI, a simulator or a code reading, and a record invented from those is worse than no record. Scenario 17 is new. Nothing in 1-16 keeps a call up long enough to see an RFC 4028 refresher, so session timers would otherwise ship with no field coverage at all — including the case the feature exists for, an intercom that loses power mid-call.
A call declined while ringing could leave the PBX with the call up while the CallKit screen cleared cleanly, and nothing in the log distinguished the two. Both halves are documented PJSUA behaviour. `pjsua_call_hangup` returns once the message is with the transaction layer — its header says the hangup process "will continue in the background" — and `pjsua_acc_del` "always deletes the account regardless of active calls", pointing at `pjsua_acc_del2` plus a wait "until the calls are fully disconnected" as the safe order. A host doing endCall() then logout() on the next line lands in exactly that window: FIFO on the SIP queue puts the 603 first, but the account is gone microseconds later, while the INVITE transaction is still waiting for an ACK. An established call is immune because a BYE is a request in an existing dialog. logout(), stop() and the account replacement inside login(configuration:) now drain `pjsua_call_get_count()` — documented to include "calls that are no longer active but still in the process of hanging up" — before deleting anything, for up to one T1 plus a round trip, and delete through pjsua_acc_del2 with force clear. unregister() is left alone: it keeps the account, which no INVITE transaction depends on. The cost is that logout() and stop() can block their caller for up to a second after a call. The teardown path had no logging whatsoever, which is what made this undiagnosable from the field. It now names the method and why it was chosen, the pj_status_t, and — through a newly wired on_call_tsx_state — the peer's side: the response going on the wire, every retransmission, the ACK, and a transaction that ended without one. That last case is the failure, and it was previously invisible. Choosing BYE vs 603 in CallWaveKit rather than leaving it to pjsua_call_hangup's zero-code inference is what makes the tsx callback usable at all: that API is documented not to deliver it. It also removes an asymmetry — the library-owned CallKit path already chose explicitly, host-owned mode did not. FIELD-TESTING scenario 4 gets the marker it was missing; its "must leave the intercom silent" could not be read off the phone.
main extracted CallWaveCallStateMachine (#22), which took `callState`, `currentCallUUID`, `currentCaller` and `microphoneMuted` out of CallWaveClient — the four properties this branch had just put behind a lock. Git kept both sides, leaving two implementations of each getter. The lock moves to where the state now lives. The client keeps its own for what stayed (`configuration`, `provider`, `running`, `registrationState`, `registrationError`, `defaultCallerName`, `pushPayloadParser`, `networkPathSummary`), and the state machine gets the same treatment for the projection, which is still read from any thread: -resolveCallForUUID: backs every argument-less call action, and the client's pass-throughs go straight to its ivars. -clearPublishedCallState is dropped in favour of -resetToIdle; -stop and -providerDidReset: keep the main-queue hop this branch added. Projection writes go through one -setProjectionUUID:caller:muted:, so a reader cannot catch the UUID and the caller name out of step. The concurrency test grew a case that drives the machine's writers directly, and it earned its keep immediately: the first version of this resolution implemented -microphoneMuted while the property declares getter=isMicrophoneMuted, so the real getter stayed auto-synthesized and unlocked. TSan reported it against the new test. Verified: 131 tests, TSan clean, device and strict-concurrency builds, podspec lint. The CHANGELOG's Unreleased section is re-sorted — the merge had put the data-race entry under Added.
This was referenced Aug 25, 2026
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.