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

When writing to STDOUT redirected to a closed pipe, no broken pipe #2532

Merged
merged 7 commits into from
Jan 18, 2022

Conversation

gogainda
Copy link
Contributor

@gogainda gogainda commented Nov 13, 2021

error message will be shown now

Ruby commit ruby/ruby@6f28ebd

before

~ % seq 100 | ruby -pe 'print if 12..' | head -2
1
2
<internal:core> core/errno.rb:54:in `handle_errno': Broken pipe (Errno::EPIPE)
	from <internal:core> core/posix.rb:449:in `write_string_native'
	from <internal:core> core/io.rb:2396:in `write'
	from <internal:core> core/kernel.rb:440:in `block in print'
	from <internal:core> core/kernel.rb:439:in `each'
	from <internal:core> core/kernel.rb:439:in `print'
	from -e:1:in `<main>'
~ % yes | ruby -pe 'print' | head -1
y
<internal:core> core/errno.rb:54:in `handle_errno': Broken pipe (Errno::EPIPE)
	from <internal:core> core/posix.rb:449:in `write_string_native'
	from <internal:core> core/io.rb:2396:in `write'
	from <internal:core> core/kernel.rb:440:in `block in print'
	from <internal:core> core/kernel.rb:439:in `each'
	from <internal:core> core/kernel.rb:439:in `print'
	from -e:1:in `<main>'

after

~ % seq 100 | ruby -pe 'print if 12..' | head -2
1
2
~ % yes | ruby -pe 'print' | head -1            
y

only problem I found is that -n is not working on truffleruby, so I don't know how it behaves with this fix

@gogainda
Copy link
Contributor Author

apparently I broke other EPIPE related tests @eregon

@eregon
Copy link
Member

eregon commented Nov 15, 2021

This is part of the Ruby 3.0 changes:

[medium] When writing to STDOUT redirected to a closed pipe, no broken pipe error message will be shown now. [Feature #14413]

@eregon eregon mentioned this pull request Nov 15, 2021
82 tasks
@gogainda
Copy link
Contributor Author

Not sure how to write test for it

@gogainda gogainda force-pushed the feature/ruby3_no_broken_pipe_msg branch from db1303d to def60be Compare November 16, 2021 23:13
@gogainda gogainda marked this pull request as ready for review November 16, 2021 23:13
@eregon
Copy link
Member

eregon commented Nov 18, 2021

@gogainda One way is to look at the CRuby test to see how it's tested. I'll write a spec for it this time.

Copy link
Member

@eregon eregon left a comment

Choose a reason for hiding this comment

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

Thank you!

@eregon eregon added this to the 22.0.0 milestone Nov 18, 2021
@eregon eregon added the in-ci The PR is being tested in CI. Do not push new commits. label Nov 18, 2021
@eregon eregon self-assigned this Nov 18, 2021
@gogainda
Copy link
Contributor Author

@eregon is there anything I can do to move this PR forward?

@eregon
Copy link
Member

eregon commented Jan 18, 2022

Sorry, I somehow lost sight of this PR, it looks like it didn't merge due to a new CI job being added and it didn't automatically run it.
I'll rebase and put it in the merge queue.

@eregon eregon force-pushed the feature/ruby3_no_broken_pipe_msg branch from 84008cd to 203ea08 Compare January 18, 2022 12:09
@eregon eregon modified the milestones: 22.0.0, 22.1.0 Jan 18, 2022
@eregon
Copy link
Member

eregon commented Jan 18, 2022

Actually there are two MRI tests failing:

  1) Failure:
TestPipe#test_stdout_epipe [/b/b/e/main/test/mri/tests/ruby/test_pipe.rb:40]:
[Errno::EPIPE] exception expected, not #<SignalException: SIGPIPE>.

  2) Failure:
TestPipe::WithConversion#test_stdout_epipe [/b/b/e/main/test/mri/tests/ruby/test_pipe.rb:40]:
[Errno::EPIPE] exception expected, not #<SignalException: SIGPIPE>.

@eregon eregon force-pushed the feature/ruby3_no_broken_pipe_msg branch from d6b5e5a to 076f00e Compare January 18, 2022 14:03
* It seems that reopen clears the flag to raise SIGPIPE instead of EPIPE,
  see oracle#2532
@eregon
Copy link
Member

eregon commented Jan 18, 2022

I'll just tag that test for now, it seems a weird edge case with STDOUT.reopen clearing the flag or so.

@graalvmbot graalvmbot merged commit 09ab4bc into oracle:master Jan 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in-ci The PR is being tested in CI. Do not push new commits. oca-signed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants