Skip to content

Conversation

qaisjp
Copy link
Collaborator

@qaisjp qaisjp commented Jul 24, 2025

subprocess.communicate("") would raise IOError: closed stream because the code closed stdin immediately when input.empty?, but then later tried to use the closed stdin file descriptor in IO.select

This only occurs with empty strings (""), not with nil or non-empty strings.

We've fixed that by excluding @stdin from wait_w when @stdin.closed?

Backward compatibility

This fix maintains full backward compatibility:

  • nil input behaviour unchanged
  • Non-empty string input behaviour unchanged
  • Only empty string input is fixed (was previously broken)

qaisjp-stripe and others added 7 commits July 24, 2025 10:57
This test demonstrates a bug in subprocess.communicate where passing an empty
string ("") causes a "closed stream" error. The issue only happens with an
empty string input, not with nil or non-empty strings.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
Previously, when passing an empty string to communicate(), stdin would be
closed immediately but then later used in IO.select calls, causing a
"closed stream" error.

This fix:
1. Only closes stdin immediately for nil input
2. For empty string input, closes stdin after determining wait_w and
   removes it from the wait_w array to prevent IO.select from using it

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
Improvements made:
1. Cleaner logic in communicate() method - explicitly handle wait_w array setup
2. Better edge case handling - check for already closed stdin
3. Improved tests that directly reproduce the original bug condition
4. Added comprehensive test coverage for nil, empty string, and edge cases

The fix now properly handles:
- Empty string input: closes stdin immediately, doesn't add to IO.select
- Nil input: closes stdin immediately (existing behavior)
- Non-empty input: adds stdin to wait_w for writing (existing behavior)
- Already closed stdin: gracefully handled without errors

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
Improves code readability by:
- Using stdin_closed variable to track state instead of repeating checks
- Maintaining clear separation between nil and empty string handling
- Eliminating repetitive @stdin.closed? checks
- Preserving original logic flow and behavior

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
Improvements:
- Move empty string handling to top with nil handling
- Use single stdin_closed variable as source of truth
- Handle pre-closed stdin edge case properly
- Simplify wait_w logic to just check \!stdin_closed

This creates cleaner, more maintainable code with the same behavior.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
@qaisjp qaisjp requested a review from Copilot July 24, 2025 16:00
Copilot

This comment was marked as outdated.

@qaisjp-stripe qaisjp-stripe force-pushed the fix-empty-string-bug branch from 86ecd92 to 642b66f Compare July 24, 2025 16:03
@qaisjp qaisjp requested a review from Copilot July 24, 2025 16:12
Copy link

@Copilot 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 fixes an IOError: closed stream bug that occurred when calling subprocess.communicate("") with an empty string input. The issue arose because the code closed stdin when input was empty but then tried to use the closed file descriptor in IO.select.

  • Fixed stdin handling logic to exclude closed stdin from IO.select operations
  • Added comprehensive test coverage for empty string, nil, and non-empty string inputs
  • Ensured backward compatibility by only fixing the broken empty string case

Reviewed Changes

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

File Description
lib/subprocess.rb Updated stdin handling to check if stdin is closed before including it in wait operations
test/test_empty_stdin.rb Added comprehensive test suite covering empty string, nil input, non-empty string, and edge cases
Comments suppressed due to low confidence (1)

test/test_empty_stdin.rb:45

  • This test creates a subprocess but doesn't handle cleanup if an assertion fails before p.wait is called. Consider wrapping in an ensure block or using the block form of check_call for automatic cleanup.
      p = Subprocess.popen(['cat'], stdin: Subprocess::PIPE, stdout: Subprocess::PIPE)

@qaisjp-stripe qaisjp-stripe merged commit e1fafcb into master Jul 24, 2025
14 of 15 checks passed
@qaisjp-stripe qaisjp-stripe deleted the fix-empty-string-bug branch July 24, 2025 16:16
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.

3 participants