Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Process.run with closed IO #14698

Merged

Conversation

straight-shoota
Copy link
Member

IOs passed to Process.run experience some indirection which blew up if an IO is closed.
This patch ensures that closed IOs are handled correctly.

  • A closed IO that is not a IO::FileDescriptor is replaced with a closed file descriptor
  • A closed file descriptor won't be reopened in the new process

Resolves part of #14569

@straight-shoota straight-shoota added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib:files labels Jun 11, 2024
@straight-shoota straight-shoota self-assigned this Jun 11, 2024
src/process.cr Show resolved Hide resolved
@straight-shoota straight-shoota added this to the 1.13.0 milestone Jun 24, 2024
@straight-shoota straight-shoota merged commit da8a9bd into crystal-lang:master Jun 25, 2024
61 checks passed
@straight-shoota straight-shoota deleted the fix/process.new-closed-io branch June 25, 2024 22:01
straight-shoota pushed a commit that referenced this pull request Nov 7, 2024
…15165)

The spec introduced in #14698 fails on Windows very rarely (e.g. https://github.com/crystal-lang/crystal/actions/runs/11681506288/job/32532033082, https://github.com/crystal-lang/crystal/actions/runs/11642959689/job/32449149741). This PR seems to fix that; other platforms already do the same in `#system_close`, and double close remains an error.
CTC97 pushed a commit to CTC97/crystal that referenced this pull request Nov 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib:system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants