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

Already on GitHub? Sign in to your account

Support IO.copy_stream from a pipe IO instance. #2114

Merged
merged 6 commits into from Feb 4, 2013

Conversation

Projects
None yet
3 participants
Contributor

warrenseen commented Jan 3, 2013

IO.copy_stream fails when the IO instance is backed by a pipe, as seek is not
supported on the underlying pipe.

This change required some modification to the copy_stream_spec, I'm open to
suggestions if there's a more preferred way of structuring the spec.

@warrenseen warrenseen Support IO.copy_stream from a pipe IO instance.
IO.copy_stream fails when the IO instance is backed by a pipe, as seek is not
supported on the underlying pipe.

This change required some modification to the copy_stream_spec, I'm open to
suggestions if there's a more preferred way of structuring the spec.
b717b95

@brixen brixen commented on an outdated diff Jan 10, 2013

spec/ruby/core/io/copy_stream_spec.rb
+ before :each do
+ @to_io = File.open @to_name, "wb"
+ end
+
+ after :each do
+ @to_io.close
+ end
+
+ it_behaves_like :io_copy_stream_to_io, nil, IOSpecs::CopyStream
+ it_behaves_like :io_copy_stream_to_io_with_offset, nil, IOSpecs::CopyStream
+ end
+ end
+
+ describe "from a pipe IO" do
+ before :each do
+ @from_io = IO.popen "cat #{@from_name}"
@brixen

brixen Jan 10, 2013

Owner

Is there a way to do this without cat?

@brixen brixen commented on an outdated diff Jan 10, 2013

spec/ruby/core/io/copy_stream_spec.rb
@@ -177,6 +183,50 @@
describe "to a file name" do
it_behaves_like :io_copy_stream_to_file, nil, IOSpecs::CopyStream
+ it_behaves_like :io_copy_stream_to_file_with_offset, nil, IOSpecs::CopyStream
+ end
+
+ describe "to an IO" do
+ before :each do
+ @to_io = File.open @to_name, "wb"
@brixen

brixen Jan 10, 2013

Owner

We have a new_io helper. Can this be used here?

@brixen brixen commented on an outdated diff Jan 10, 2013

kernel/common/io19.rb
@@ -274,7 +274,11 @@ def run
@from.ensure_open_and_readable
@to.ensure_open_and_writable
- saved_pos = @from.pos if @from_io
+ begin
+ saved_pos = @from.pos if @from_io
+ rescue Errno::ESPIPE
@brixen

brixen Jan 10, 2013

Owner

Could we do this with a flag on IO instead of rescuing an exception? Causing exceptions in the normal flow is very bad for performance.

Owner

dbussink commented Jan 23, 2013

Do you think you could improve the pull request with the feedback provided? If there's stuff unclear, please let us know!

Contributor

warrenseen commented Jan 23, 2013

@dbussink yep, I've got it sitting here nearly done, hoping to get a chance to finish it off over the weekend.

@warrenseen warrenseen Add pipe flag to IO.
Add a pipe flag to IO class so we can avoid calling #seek on the source IO in copy_stream.
2c7a44b
Contributor

warrenseen commented Jan 29, 2013

@dbussink, it took me a little longer to get around to it, but I've replaced the call to cat with something that should be OS independent but still gives us a pipe to work with.

@brixen brixen merged commit 36dc79f into rubinius:master Feb 4, 2013

1 check failed

default The Travis build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment