[PHP-wasm] Add support for concurrent popen calls#3486
Open
bgrgicak wants to merge 4 commits intowith-smtp-sinkfrom
Open
[PHP-wasm] Add support for concurrent popen calls#3486bgrgicak wants to merge 4 commits intowith-smtp-sinkfrom
bgrgicak wants to merge 4 commits intowith-smtp-sinkfrom
Conversation
js_popen_store_pid -> js_popen_set_pid_for_fd js_popen_lookup_pid -> js_popen_get_pid_for_fd js_popen_remove_fd -> js_popen_clear_pid_for_fd Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Verifies that pclose returns the correct exit code for each process when multiple popen handles are open simultaneously. Also restricts concurrent popen tests to PHP 8.4 until all versions are recompiled. Co-Authored-By: Claude Opus 4.6 (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.
Motivation for the change, related issues
The previous
popen/pcloseimplementation used single global variables (wasm_popen_last_pidandwasm_pclose_ret) to track the spawned process PID and exit code. This meant concurrent writablepopen()calls would clobber each other's state, producing wrong exit codes or closing the wrong process.Additionally,
popen/pcloseinterception relied on aVCWD_POPENmacro patch and explicitsedreplacements inmail.c, which was fragile and didn't cover all call sites.Implementation details
Linker-level wrapping replaces macro patching
Instead of patching
VCWD_POPENinzend_virtual_cwd.handsed-replacingpopen/pcloseinmail.c, this PR uses--wrap=popenand--wrap=pcloselinker flags. This transparently redirects allpopen()/pclose()calls in the compiled C code to__wrap_popen/__wrap_pclose, which delegate towasm_popen/wasm_pclose. Forpclose, handles not created bywasm_popenfall through to the real libcpclose.Per-fd PID tracking instead of globals
Replaces
wasm_popen_last_pidandwasm_pclose_retwith an fd-to-pid mapping stored in the existing JSPHPWASM.processTable. Three new Emscripten library functions manage the mapping:js_popen_set_pid_for_fd(fd, pid)— called bywasm_popenafter spawning a processjs_popen_get_pid_for_fd(fd)— called bywasm_pcloseto find which process to wait onjs_popen_clear_pid_for_fd(fd)— called bywasm_pclosebefore closing the fileThis allows multiple
popenhandles to be open simultaneously without state corruption.Emscripten library updates
Adds
__wrap_popen,__wrap_pclose, andzif_pcloseto the Asyncify import list and JSPI export list so the new wrapper functions can suspend correctly.TODO
Testing Instructions