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

child_process helper: fix stderr blocking for discard case. fix #2609 #2649

Merged
merged 2 commits into from
Oct 15, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions lib/fluent/plugin_helper/child_process.rb
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,8 @@ def child_process_execute_once(
readio = writeio = stderrio = wait_thread = nil
readio_in_use = writeio_in_use = stderrio_in_use = false

if !mode.include?(:stderr) && !mode.include?(:read_with_stderr) && stderr != :discard # connect
if !mode.include?(:stderr) && !mode.include?(:read_with_stderr)
spawn_opts[:err] = IO::NULL if stderr == :discard
writeio, readio, wait_thread = *Open3.popen2(*spawn_args, spawn_opts)
elsif mode.include?(:read_with_stderr)
writeio, readio, wait_thread = *Open3.popen2e(*spawn_args, spawn_opts)
Expand All @@ -281,7 +282,7 @@ def child_process_execute_once(
stderrio.set_encoding(external_encoding, internal_encoding, encoding_options)
stderrio_in_use = true
else
stderrio.reopen(IO::NULL) if stderrio && stderrio == :discard
Copy link
Member

Choose a reason for hiding this comment

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

Just checking, This was a bug, right? stderrio cannot be a :discard.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. This is typo.

stderrio.reopen(IO::NULL) if stderrio && stderr == :discard
end

pid = wait_thread.pid # wait_thread => Process::Waiter
Expand Down
37 changes: 37 additions & 0 deletions test/plugin_helper/test_child_process.rb
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,43 @@ def configure(conf)
assert_equal expected, ary
end

test 'can execute external command at just once, which can handle both of read and write. Ignore stderr message/no block' do
m = Mutex.new
ary = []
Timeout.timeout(TEST_DEADLOCK_TIMEOUT) do
ran = false
# lots of stderr message should not be blocked and message should not be printed in test.
cmd = "ruby -e 'while !STDIN.eof? && line = STDIN.readline; STDERR.puts line.chomp * 1000; STDOUT.puts line.chomp; STDOUT.flush rescue nil; end'"
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure but it needs to callSTDERR.flush?

Copy link
Member Author

Choose a reason for hiding this comment

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

STDERR writes lots of data so no flush call is not critical.

@d.child_process_execute(:t2_and_ignore_stderr, cmd, mode: [:write, :read]) do |writeio, readio|
m.lock
ran = true

[[1,2],[3,4],[5,6]].each do |i,j|
writeio.write "my data#{i}\n"
writeio.write "my data#{j}\n"
writeio.flush
end
writeio.close

while line = readio.readline
ary << line
end
m.unlock
end
begin
sleep TEST_WAIT_INTERVAL_FOR_BLOCK_RUNNING until m.locked? || ran
m.lock
rescue
ensure
m.unlock
end
end

assert_equal [], @d.log.out.logs
expected = (1..6).map{|i| "my data#{i}\n" }
assert_equal expected, ary
end

test 'can execute external command at just once, which can handle all of read, write and stderr' do
m = Mutex.new
ary1 = []
Expand Down