Skip to content

Refactor internal Crystal::System::Process#fork on UNIX#16191

Merged
straight-shoota merged 1 commit intocrystal-lang:masterfrom
ysbaddaden:refactor/unix-system-process-fork
Oct 30, 2025
Merged

Refactor internal Crystal::System::Process#fork on UNIX#16191
straight-shoota merged 1 commit intocrystal-lang:masterfrom
ysbaddaden:refactor/unix-system-process-fork

Conversation

@ysbaddaden
Copy link
Collaborator

The method now yields to let the caller do something in the child process after fork, rather than always compiling both paths (fork vs fork/exec) even though most applications only use the fork/exec path.

Extracted from #16174.

The method now yields to let the caller do something in the child
process after fork, rather than always compiling both paths (fork vs
fork/exec) even though most applications only use the fork/exec path.
@straight-shoota
Copy link
Member

I suppose this is fine as an intermediary.

We're planning to stop using fork for spawning a process and use posix_spawn instead (see #11337), or a custom implementation using clone on linux. At that point the current fork method will only be used for the deprecated Process.fork.

@ysbaddaden ysbaddaden added this to the 1.19.0 milestone Oct 28, 2025
@straight-shoota straight-shoota merged commit 4da79d8 into crystal-lang:master Oct 30, 2025
41 checks passed
@ysbaddaden ysbaddaden deleted the refactor/unix-system-process-fork branch October 30, 2025 14:59
@straight-shoota
Copy link
Member

straight-shoota commented Nov 15, 2025

This change broke Process.fork (without a block):

Process.fork # Error: 'Crystal::System::Process.fork' is expected to be invoked with a block, but no block was given

Process.fork calls the system method directly which now requires a will_exec parameter.
There seems to be no spec for this whatsoever 🤦

straight-shoota added a commit that referenced this pull request Nov 18, 2025
Rename the internal method to `system_fork` so we clearly separate it from the method that the public API and the compiler use.

Fixes #16191 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants