Emulate POSIX_SPAWN_CLOEXEC_DEFAULT in fork/exec #79
Conversation
iCharlesHu
force-pushed
the
charles/fork-exec-close-fd
branch
from
6e9afb5 to
3e47e05
Compare
iCharlesHu
force-pushed
the
charles/fork-exec-close-fd
branch
from
3e47e05 to
2d9a937
Compare
| hi = getdtablesize(); | ||
| } | ||
| #else | ||
| int hi = 1024; |
There was a problem hiding this comment.
Where does '1024' come from and how safe is it to default to this vs making this #error and requiring a specific implementation for new platforms?
Also, the following additional platforms support getdtablesize: FreeBSD, OpenBSD, NetBSD, Solaris, QNX.
There was a problem hiding this comment.
Instead of #error, how about if I just call getdtablesize unconditionally here and have it naturally error on platforms that don't support it.
|
FreeBSD PR #1698 adds support for POSIX O_CLOFORK, which could simplify this implementation. |
Ahh thanks. Good to know. |
iCharlesHu
force-pushed
the
charles/fork-exec-close-fd
branch
from
2d9a937 to
cd79141
Compare
| char * _Nullable const args[_Nonnull], | ||
| char * _Nullable const env[_Nullable], | ||
| gid_t * _Nullable process_group_id | ||
| int _shims_snprintf( |
There was a problem hiding this comment.
On Linux this function isn't exposed via import Glibc
iCharlesHu
force-pushed
the
charles/fork-exec-close-fd
branch
from
cd79141 to
c217b39
Compare
|
Addressed review comments. |
|
Just a heads up - the PR has merged. I think the timing means it'll likely make it into the upcoming FreeBSD 15 release in December. |
There was a problem hiding this comment.
The Linux CI failure looks relevant
[7/22] Compiling CSystem shims.c
/__w/swift-subprocess/swift-subprocess/Sources/_SubprocessCShims/process_shims.c:318:28: error: use of undeclared identifier 'val'
318 | if (errno == ERANGE || val <= 0 || val > INT_MAX) {
|
|
Now that O_CLOFORK is merged in all *BSD, work can be started on support O_CLOFORK flags. |
iCharlesHu
force-pushed
the
charles/fork-exec-close-fd
branch
4 times, most recently
from
d78a5c8 to
6176f07
Compare
| ) | ||
| // Depending on the platform, standard error should be something like | ||
| // `/bin/bash: 7: Bad file descriptor | ||
| #expect(!result.standardError!.isEmpty) |
There was a problem hiding this comment.
nit: avoid force unwraps:
| #expect(!result.standardError!.isEmpty) | |
| let standardError = try #require(result.standardError) | |
| #expect(!standardError.isEmpty) |
POSIX_SPAWN_CLOEXEC_DEFAULT is only available on Darwin. Emulate POSIX_SPAWN_CLOEXEC_DEFAULT on other platforms by calling close after fork, before exec. This commit also removes _subprocess_posix_spawn_fallback because we can't emulate POSIX_SPAWN_CLOEXEC_DEFAULT in a thread-safe manner while using posix_spawn.
iCharlesHu
force-pushed
the
charles/fork-exec-close-fd
branch
from
6176f07 to
707b001
Compare
|
Resolves: #132 |
iCharlesHu
force-pushed
the
charles/fork-exec-close-fd
branch
from
707b001 to
b2e57ea
Compare
|
✘ Test testDoesNotReapUnrelatedChildProcess() recorded an issue at ProcessMonitoringTests.swift:229:6: Caught error: Failed to spawn the new process with underlying error: UnderlyingError(rawValue: 11) |
There was a problem hiding this comment.
Looks good to me!
Note that there is one test failure:
✘ Test stressTestWithLittleOutput() recorded an issue at IntegrationTests.swift:1457:6: Caught error: Failed to read bytes from the child process with underlying error: UnderlyingError(rawValue: 17)
...which is EEXIST, and noble is hanging somewhere.
But you could probably just re-run the CI to work around that and address those issues separately.
8 participants
POSIX_SPAWN_CLOEXEC_DEFAULT is only available on Darwin. Emulate POSIX_SPAWN_CLOEXEC_DEFAULT on other platforms by calling close after fork, before exec.
This commit also removes _subprocess_posix_spawn_fallback because we can't emulate POSIX_SPAWN_CLOEXEC_DEFAULT in a thread-safe manner while using posix_spawn.
Resolves: #46
Resolves: #132