Skip to content

Close stdin immediately when using popen2e#9540

Merged
hsbt merged 1 commit into
ruby:masterfrom
Shopify:rwstauner/stdin-close
May 14, 2026
Merged

Close stdin immediately when using popen2e#9540
hsbt merged 1 commit into
ruby:masterfrom
Shopify:rwstauner/stdin-close

Conversation

@rwstauner
Copy link
Copy Markdown
Contributor

It's good hygiene to close the stdin pipe as soon as you are done writing to it.

This can happen for example if "ruby extconf.rb" spawns another process (for example "cargo build", which may spawn arbitrary commands to fetch credentials) and any of those subprocesses attempt to read STDIN until it is closed.

We can close it as soon as it's created since we aren't writing to it at all.

What was the end-user or developer problem that led to this PR?

Our CI was hanging and timing out at the "bundle install" step because a gem's extconf.rb calls cargo build which calls a custom command to fetch credentials and that command tries to read STDIN when it detects that it is given a pipe.

What is your fix for the problem, implemented in this PR?

Close the STDIN pipe created by Open3 so that if a target command tries to read the pipe it will finish.

Make sure the following tasks are checked

Comment thread test/rubygems/test_gem_ext_builder.rb Outdated
@rwstauner rwstauner force-pushed the rwstauner/stdin-close branch from d4062de to 5b3f795 Compare May 13, 2026 18:53
@kou kou requested a review from Copilot May 14, 2026 00:35
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates Gem::Ext::Builder.run to proactively close the child process STDIN pipe created by Open3.popen2e, preventing subprocess trees from blocking on STDIN reads during native extension builds (e.g., extconf.rbcargo build → credential helper).

Changes:

  • Close the stdin IO immediately inside the Open3.popen2e block in Gem::Ext::Builder.run.
  • Add a regression test asserting that Gem::Ext::Builder.run provides an immediately-EOF STDIN to the child process.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
lib/rubygems/ext/builder.rb Closes stdin immediately after spawning the build command to avoid hangs from downstream STDIN reads.
test/rubygems/test_gem_ext_builder.rb Adds a test that verifies STDIN is closed (EOF) for commands run via Gem::Ext::Builder.run.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +119 to +123
Gem::Ext::Builder.run([Gem.ruby, "-e", check_stdin_script], results)

command_output = results.last
assert_equal "STDIN: \"\"\n", command_output
end
@hsbt hsbt enabled auto-merge (rebase) May 14, 2026 00:42
@hsbt hsbt disabled auto-merge May 14, 2026 00:42
Copy link
Copy Markdown
Member

@kou kou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@hsbt
Copy link
Copy Markdown
Member

hsbt commented May 14, 2026

@rwstauner Thanks always. I'm not sure why realworld-bundler CI status is stucked. Could you rebase and push again for merging this?

@rwstauner rwstauner force-pushed the rwstauner/stdin-close branch from 5b3f795 to 69a4ff1 Compare May 14, 2026 18:06
It's good hygiene to close the stdin pipe as soon as you are done writing to it.

This can happen for example if "ruby extconf.rb" spawns another
process (for example "cargo build", which may spawn arbitrary commands
to fetch credentials) and any of those subprocesses attempt to read
STDIN until it is closed.

We can close it as soon as it's created since we aren't writing to it at all.
@rwstauner rwstauner force-pushed the rwstauner/stdin-close branch from 69a4ff1 to ab09bfd Compare May 14, 2026 19:23
@rwstauner
Copy link
Copy Markdown
Contributor Author

I rebased and it's green now.
Thank you!

@hsbt hsbt merged commit 5869518 into ruby:master May 14, 2026
93 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants