Repository navigation
Make the wasm lookup cache, the session status and the main thread id safe for concurrent commands - #195
Open
demrom wants to merge 3 commits into
Open
Make the wasm lookup cache, the session status and the main thread id safe for concurrent commands#195demrom wants to merge 3 commits into
demrom wants to merge 3 commits into
Conversation
…still owns it ios_storeThreadId stores a new thread only if thread_ids[current_pid] is -1, and current_pid is a global. A command that ends between ios_fork and that store rewound current_pid, so the new thread checked the wrong slot, skipped the store, and ios_waitpid never returned. The three release sites now rewind current_pid with a compare-and-swap, only if the ending command is still the current one. tests/concurrent-fork.m runs eight threads that each fork, run a command and wait for it 400 times. A two-minute alarm turns a hang into a failure and prints the stacks of all threads.
… safe for concurrent commands ThreadSanitizer reports three races between command threads: the cached dlsym result in executeWebAssemblyCommandsInOrder, the session's global_errno, and the session's mainThreadId. The first two are atomic now. mainThreadId is set and cleared with compare-and-swap and loaded once before pthread_kill. mainThreadId is recorded on the command thread itself, in run_function, so a command that ends before its starter returns is never recorded after it ended. tests/main-thread-winch.m resizes the window while a long command runs after a short one. tests/tsan.supp hides the accesses inside the window where ios_storeThreadId unlocks pid_mtx on another thread, which TSan cannot follow. tests/README.md says how to build and run with it.
demrom
marked this pull request as ready for review
October 24, 2026 04:17
Owner
|
Thanks a lot for that one. |
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
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.
Depends on #192; until it lands this diff also shows its commits.
When commands run from several threads, ThreadSanitizer reports three data races, all of them real:
executeWebAssemblyCommandsInOrdercaches the result ofdlsymin astaticfunction pointer that command threads write and read without synchronization. It is now_Atomic, and the lookup still falls back toRTLD_DEFAULT.global_errnois written by every command thread and read by the caller ofios_system. It is now_Atomic int.mainThreadIdis written byios_systemwhen it starts the first command of a session, and read and cleared bycleanup_functionon the command thread. It is now_Atomic(pthread_t). The command thread stores its own id inrun_function(a newstoreMainThreadflag), withatomic_compare_exchange_strongonly while the field is empty, so a command that ends beforeios_systemreturns is never recorded after it ended.cleanup_functionclears it withatomic_compare_exchange_strongonly while it still holds its own thread, and the window-resize code loads it once beforepthread_kill.TSan also reports
ios_nextAvailablePidandios_storeThreadId: the first returns withpid_mtxlocked and the second unlocks it on the new command thread, which TSan cannot follow, so it reports two threads holding the mutex at once. That locking stays as it is.tests/tsan.suppsuppresses the accesses inside that window (ios_nextAvailablePid,storeEnvironment,environmentVariables,resetEnvironment),ios_restoreParentPid, whose compare-and-swap on the release path races with the plain writes in that window, andios_currentPid. Theios_currentPidentry also hides the unlocked reads ofcurrent_pidagainst the plain writes inios_nextAvailablePid, which this change leaves alone.tests/README.mdexplains how to build withENABLE_THREAD_SANITIZER=YESand run with the file.Test
tests/concurrent-fork.m, linked with-fsanitize=threadagainst a TSan build of the framework from a fresh clone of this branch and run withsuppressions=tests/tsan.suppon the iOS 27 Simulator:WARNING: ThreadSanitizerreports per run in 10 of 10 runs, onexecuteWebAssemblyCommandsInOrder(the cached lookup) andrun_function(theglobal_errnowrite).8 threads x 400 commands PASS.tests/main-thread-winch.mcovers the stale id: a short command ends beforeios_systemreturns, then a long command installs aSIGWINCHhandler and the test callsios_setWindowSizeand checks that the handler runs on the long command's thread. On master + #192, 5 of 5 runs fail withSIGWINCH was handled on another thread; with this change 10 of 10 runs pass.With the other four changes of the series merged, TSan also reports the
mainThreadIdwriteios_systemmakes after creating the command (in__ios_system_block_invoke) against the clear incleanup_function. Without themainThreadIdpart of this change,concurrent-forkreports it in 5 of 10 runs andconcurrent-getoptin 1 of 10, one report each; with it, 0 reports in 10 of 10 runs of each.On an iPhone, in a-Shell mini 2.2.3 (592) built from a-shell master, with an
ios_systembuilt from all five changes of this series merged, 120 pipelines ofcat | sort -r -u -n | uniq -c | head -n 3ran one after another from dash, each with the expected output. The races are timing-dependent, so the Simulator TSan runs above are what show them fixed.