Skip to content

Commit

Permalink
Fixed an issue with writing multi-byte characters to the IO stdin str…
Browse files Browse the repository at this point in the history
…eam.

NoMethodError You have a nil object when you didn't expect it!
albino (1.2.3) lib/albino/process.rb:167:in block in read_and_write
  • Loading branch information
raid5 committed Feb 2, 2011
1 parent 1219478 commit c115c80
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 9 deletions.
13 changes: 4 additions & 9 deletions lib/albino/process.rb
Original file line number Diff line number Diff line change
Expand Up @@ -158,16 +158,11 @@ def read_and_write(input, stdin, stdout, stderr, timeout=nil, max=nil)
# write to stdin stream
ready[1].each do |fd|
begin
boom = nil
size = fd.write_nonblock(input)
input = input[size, input.size]
rescue Errno::EPIPE => boom
rescue Errno::EAGAIN, Errno::EINTR
end
if boom || input.size == 0
stdin.close
writers.delete(stdin)
fd.write(input)
rescue Errno::EPIPE, Errno::EAGAIN, Errno::EINTR

This comment has been minimized.

Copy link
@nono

nono Feb 2, 2011

For Errno::EAGAIN and Errno::EINTR, I think you should not do the:

stdin.close
writers.delete(stdin)

This comment has been minimized.

Copy link
@nono

nono Feb 2, 2011

Hum, after reading more closely the Ruby documentation, I don't see a reason to try to rescue the exception with write (but the begin rescue block should be kept for Errno::EPIPE).

This comment has been minimized.

Copy link
@raid5

raid5 Feb 3, 2011

Author Owner

Were Errno::EAGAIN and Errno::EINTR needed for the original code that used write_nonblock?

This comment has been minimized.

Copy link
@nono

nono Feb 3, 2011

Yes, write_nonblock can return Errno::EAGAIN and Errno::EINTR, but write doesn't according to the doc.

end
stdin.close
writers.delete(stdin)
end

# read from stdout and stderr streams
Expand Down
7 changes: 7 additions & 0 deletions test/process_test.rb
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
# encoding: UTF-8
require 'albino'
require 'rubygems'
require 'test/unit'
Expand Down Expand Up @@ -48,6 +49,12 @@ def test_input
p = Albino::Process.new(['wc', '-l'], {}, :input => input)
assert_equal 100_000, p.out.strip.to_i
end

def test_utf8_input
input = "smörgåsbord\n" * 100_000
p = Albino::Process.new(['wc', '-l'], {}, :input => input)
assert_equal 100_000, p.out.strip.to_i
end

def test_max
assert_raise Albino::Process::MaximumOutputExceeded do
Expand Down

3 comments on commit c115c80

@nono
Copy link

@nono nono commented on c115c80 Feb 2, 2011

Choose a reason for hiding this comment

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

The output string is encoded as "ascii", I have to use force_encoding("utf-8") to make it works in my Rails app. I hope you see how to fix that ;-)

@raid5
Copy link
Owner Author

@raid5 raid5 commented on c115c80 Feb 3, 2011

Choose a reason for hiding this comment

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

I didn't mess with the output at all, did I? :/ This patch was just addressing stdin. Should that fix be a different patch/pull request?

@nono
Copy link

@nono nono commented on c115c80 Feb 3, 2011

Choose a reason for hiding this comment

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

I think it's a regression between Albino 1.1.1 and 1.2.3, not from your patch.

Please sign in to comment.