Serialise pty allocation: forkpty is not thread safe on macOS - #7
Merged
Merged
Conversation
Concurrent calls to pty_spawn fail outright on macOS, and often. A pure-C harness with 24 threads calling pty_spawn at once failed 5 of 24 on three consecutive runs, and 0 of 24 on three consecutive runs with nothing changed but the mutex added here. No .NET involved in either measurement. Darwin's forkpty() -> openpty() -> grantpt()/unlockpt() path is the part that is not reentrant. It is unpleasant to diagnose because it reports no usable errno: forkpty returns -1 and leaves errno at -6. NEGATIVE, so not a POSIX errno at all but a kernel-style -ENXIO leaking out of the pty allocator. strerror renders it "Undefined error: 0", and anything reading it as an errno concludes something false. It also looks exactly like fd exhaustion and is not -- measured during a failing run, the process held 30 open descriptors against a limit of 1048576, and the system had 31 of 511 ptys in use. Nothing was exhausted. In practice this is not a test-only concern: opening several terminals at once on macOS failed about one time in five. The lock is held across fork(), which is safe here for the narrow reason that the child never touches it: on every path the child either execvp()s or _exit()s, so its copy of the mutex dies with it. Only the parent unlocks, hence the pid != 0 test. errno is captured before the unlock, since pthread_mutex_unlock may clobber it. Applied unconditionally rather than ifdef'd for Darwin: glibc's openpty does not appear to need it, and the lock costs microseconds on a path that is already forking a process. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 21, 2026
There was a problem hiding this comment.
🟡 Changes recommended
Portable pthread linkage and concurrency regression coverage are missing.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Serializes native PTY allocation to prevent concurrent forkpty() failures on macOS.
Changes:
- Adds a process-wide mutex around
forkpty(). - Preserves
errnobefore unlocking.
File summaries
| File | Description |
|---|---|
src/Porta.Pty.Native/porta_pty.c |
Serializes PTY spawning. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+161
to
162
| pthread_mutex_lock(&pty_spawn_lock); | ||
| pid_t pid = forkpty(&master_fd, NULL, term_ptr, ws_ptr); |
| #include <string.h> | ||
| #include <unistd.h> | ||
| #include <errno.h> | ||
| #include <pthread.h> |
tomlm
approved these changes
Aug 22, 2026
JohnCampionJr
added a commit
to JohnCampionJr/Porta.Pty
that referenced
this pull request
Aug 22, 2026
… found it all Answers #33. Everything here came out of driving this library hard from a desktop app — terminal panes and long-running children, many at once — so each piece is a measurement rather than a preference. OUT-OF-BAND ConPTY. Microsoft ships ConPTY out of band as Microsoft.Windows.Console.ConPTY (conpty.dll + OpenConsole.exe), the same implementation Windows Terminal carries. Both are wired up behind PORTAPTY_CONPTY so the choice can be measured rather than argued; out-of-band is the default with an automatic fallback to in-box when conpty.dll is not beside the assembly, so a consumer who has not referenced the package cannot break. It appeared for a long time to cost ~3.0 SECONDS per pseudoconsole. It does not, and the reason is worth the docs page: ConPTY asks the terminal what it is (Primary Device Attributes) and blocks three seconds waiting for a reply that a read-only consumer never sends. Answering it up front took first-output latency from [3016,3012,3011,3013,3019]ms to [15,9,9,8,8]ms. Out-of-band then measures 9ms per pseudoconsole against in-box's 13ms. Two things that made the A/B lie before it told the truth, both in docs: recent Windows 11 ships its own System32\conpty.dll, so an unqualified DllImport resolves the OS copy and the 'out-of-band' arm is quietly in-box; and the only direct evidence of which implementation is live is a process census, because in-box spawns conhost.exe per pseudoconsole and out-of-band spawns OpenConsole.exe. CsWin32 REPLACES Vanara. The point is the runtime dependency: Vanara ships an assembly every consumer then carries, for about twenty entry points. CsWin32 is a build-time generator with PrivateAssets=all, so it contributes nothing at runtime and nothing to a consumer's graph, and the generated interop is trimming and AOT friendly. The idea is Sylinko's, from their fork; this is an independent implementation of it. WINDOWS JOB-OBJECT RACE. CreateProcessW was not given CREATE_SUSPENDED, so a child could run — and exit — before AssignProcessToJobObject reached it, failing with 'Failed to assign process to job object'. Created suspended, assigned, then resumed. net10.0 and MSTest on the Microsoft Testing Platform. MTP reports crashes and hangs instead of absorbing them, which matters here because almost everything interesting is threads and process lifetime — a swallowed hang looks exactly like a pass. ConcurrentSpawnTests is the harness the rest came from: 24 concurrent spawns, 20 samples per cell, minimal and realistic shells. It is what showed the reader strategy dominating everything else (137ms to first output on a dedicated thread against 7546ms pooled) and what surfaced forkpty's thread-unsafety on macOS as 5/24 failures rather than as an occasional mystery. Overlaps tomlm#7, tomlm#8 and tomlm#9 — those are the same fixes against the pre-CsWin32 tree. Merge them first and I will rebase, or take this and close them, whichever you prefer. 23 tests, 0 failed on macOS (3 Windows-only skipped). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Concurrent calls to
pty_spawnfail outright on macOS, and often.A pure-C harness with 24 threads calling
pty_spawnat once, no .NET involved:Darwin's
forkpty()→openpty()→grantpt()/unlockpt()path is the part that isn't reentrant.Why it's nasty to diagnose
It reports no usable errno.
forkptyreturns -1 and leaveserrnoat -6 — negative, so not a POSIX errno at all but a kernel-style-ENXIOleaking out of the pty allocator.strerrorrenders it"Undefined error: 0", and anything reading it as an errno concludes something false.It also looks exactly like fd exhaustion, and isn't: measured mid-failure, the process held 30 open descriptors against a limit of 1048576, and the system had 31 of 511 ptys in use. Nothing was exhausted.
In practice this isn't test-only — opening several terminals at once on macOS failed about one time in five.
On the lock across
fork()Safe here for a narrow reason: the child never touches it. On every path the child either
execvp()s or_exit()s, so its copy of the mutex dies with it. Only the parent unlocks, hence thepid != 0test.errnois captured before the unlock, sincepthread_mutex_unlockmay clobber it.Applied unconditionally rather than
#ifdef'd for Darwin — glibc'sopenptydoesn't appear to need it, and the lock costs microseconds on a path that is already forking a process.Found while using Porta.Pty to drive terminal panes in a desktop app. There are a few other fixes in my fork — happy to send any of them as separate PRs if useful, or leave them.