Skip to content

Make a caller wait for a free pid when all of them are taken - #194

Merged
holzschu merged 3 commits into
holzschu:masterfrom
demrom:fix/pid-admission
Nov 2, 2026
Merged

holzschu merged 3 commits into
holzschu:masterfrom
demrom:fix/pid-admission

Conversation

@demrom

@demrom demrom commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

Depends on #192; until it lands this diff also shows its commits, and the new test needs it to finish.

ios_nextAvailablePid hands out one of 127 pids. With all of them taken, the caller looped over the pid table while holding pid_mtx, which ending commands need to free their slot (cleanup_function spins on pthread_mutex_trylock(&pid_mtx)). No slot was ever freed: the other callers blocked in pthread_mutex_lock, the ending commands spun, and ios_waitpid never returned.

A caller that finds no free slot now releases pid_mtx and sleeps on the condition variable ios_waitpid already uses, and the broadcast an ending command already sends wakes it. Cancellation is disabled around that wait, since a thread cancelled in pthread_cond_wait would keep the mutex every ending command takes. Once awake, the caller retakes pid_mtx, rereads the parent command's environment (ios_setenv may have reallocated it meanwhile) and looks for a free slot again.

Test

tests/pid-admission.m starts 200 threads that each run ios_fork, ios_system("hold") and ios_waitpid. hold blocks until the test releases it, so after 127 commands start, the other 73 callers wait for a pid and nothing ends. The test fails if the process uses more than 0.5 s of CPU over the next second: sleeping callers use none, while a caller that loops on the pid table uses a whole core. A watchdog prints the stacks of all threads (tests/hang-dump.h) and exits 124 after 10 seconds without progress.

On the iOS 27 Simulator:

dash in a-Shell never holds 127 pids at once: a pipeline takes one pid, and a & job runs to completion before the next one starts (evalsubshell in dash_iOS src/eval.c). So the pid limit is covered by this Simulator test only.

With every pid in use, ios_nextAvailablePid looped over the table while holding pid_mtx, and the ending commands needed pid_mtx to free their pid, so none was ever freed and ios_waitpid never returned.

The caller now gives pid_mtx back and waits on the thread completion condition until a pid is free.

tests/pid-admission.m runs 200 callers over 127 pids. It measures the CPU the waiting callers use while the table is full, and a watchdog turns a deadlock into a failure with the stacks of all threads.

Cancellation is disabled while the caller waits, so a thread cancelled there does not keep thread_completion_mutex.
@demrom
demrom marked this pull request as ready for review October 24, 2026 04:17
@holzschu

holzschu commented Nov 1, 2026

Copy link
Copy Markdown
Owner

That fix makes sense, but it got me wondering: is that an issue that you actually met (and in that case, would it make sense to increase the limit to a higher value? and if so which one?)

@demrom

demrom commented Nov 2, 2026

Copy link
Copy Markdown
Contributor Author

Yes, in a stress test, not in normal use. I ran 160 commands at once from separate threads on the Simulator, and all 12 configurations I tried without this change froze: the stacks showed the caller spinning in ios_nextAvailablePid while the ending commands looped on pid_mtx in cleanup_function. tests/pid-admission.m reproduces it with 200 threads.

I'd keep the limit at 128. With this change a caller past the limit waits for a free pid instead of hanging, so the limit only caps how many commands run at the same time. Raising it is cheap: a slot costs about 1 KB of static memory, almost all of it previousDirectory[MAXPATHLEN], so going from 128 to 256 slots adds about 135 KB.

@holzschu

holzschu commented Nov 2, 2026

Copy link
Copy Markdown
Owner

Thanks for the explanation.

@holzschu
holzschu merged commit 3c1ed63 into holzschu:master Nov 2, 2026
2 checks passed
@demrom
demrom deleted the fix/pid-admission branch November 2, 2026 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants