Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Close input streams when sending commands that don't read input. #246

Merged
merged 1 commit into from

2 participants

@dylanahsmith

Fixes issue #245

@dylanahsmith dylanahsmith Close input streams when sending commands that don't read input.
An :eof option is added to the run method so that this can be specified
explicitly where a block is provided that may or may not need to read from
the input stream.
784c98a
@carsomyr

Can a command ever get anything useful from stdin?

@dylanahsmith

In our specific case we are sending a list of files to upload. However, as a workaround we are currently doing a put to a temporary file, then specifying that file as a command line argument.

In the general case, standard input is better to use if a lot of data is being sent, since it is streamed and the number or size of command line arguments is limited.

@carsomyr

Is ch.eof! interpreted by the remote SSH session or the application running inside it? Why wouldn't this be the default behavior? I am wondering about cases where one would want the remote process stdin open.

@dylanahsmith

ch.eof! is being executed locally on the SSH session, and it results in SSH closing the input stream on the remote command.

I avoided changing the default behaviour to avoid breaking existing recipes. However, the input stream is closed where it is known that the recipe won't be able to send data over the input stream.

Reading from stdin is currently being done in capistrano itself. Capistrano::Deploy::Strategy::Remote#scm_run runs the command, parses the output of the command using handle_data to detect prompts for input, and uses ch.send_data to respond to the prompts over the input stream.

@carsomyr

Ok, here's my understanding so far: If the processing is totally synchronous, then ch.eof! is perfectly safe, because by the time it executes, all interactions of Capistrano with the remote process are finished. However, if the processing is asynchronous, then ch.eof! might not be such a great idea.

@dylanahsmith

If I understand your concern correctly, it sounds like you are worried that the input stream might be closed before all the data from the send_data call is sent. I think the documentation for eof! makes it clear that the stream will only be closed after all the data is sent:

Tells the remote end of the channel that no more data is forthcoming from this end of the channel. The remote end may still send data. The CHANNEL_EOF packet will be sent once the output buffer is empty.

@carsomyr

@dylanahsmith I have some questions for you.

  1. What about the :eof => true case when a block is given? No effect, I assume? Perhaps it's better to raise an exception.
  2. When a block is given, are the semantics like IO.popen? I'm not familiar with this part of Capistrano.
  3. Is :eof => true by default when a string is given?
@dylanahsmith

What about the :eof => true case when a block is given? No effect, I assume? Perhaps it's better to raise an exception.

The input stream will be closed in this case. The block may only be used for reading the output of the command, with no intentions of sending data to the command. In fact, this is what the capture command is doing internally with this pull request.

When a block is given, are the semantics like IO.popen? I'm not familiar with this part of Capistrano.

The block is an on_data callback, which will be called whenever output is received from the command on any of the servers. This is very different from IO.popen, which calls the block once right away and closes the pipe afterwards.

Is :eof => true by default when a string is given?

I assume you mean through the :data option. The default is only based on whether a block is given, the :data option doesn't determine the default for whether the input stream should be closed. This way data can be sent to a command before any output is received, because the block is only called after receiving output from the command.

@carsomyr

I thought about this, and your reasoning is sound. I have two more questions.

  1. The ch.eof! statement is given at the initialization phase of the connection, which then goes into an IO.select loop that calls back to the block. If a block isn't provided, that means we didn't want to take advantage of this loop anyways, and so it might as well be the case that :eof => true. Is this correct?
  2. How did you mock the unit test?
@dylanahsmith

If a block isn't provided, that means we didn't want to take advantage of this loop anyways, and so it might as well be the case that :eof => true. Is this correct?

There is a default_io_proc that is reading the output of the command and logging it. However, the default_io_proc never sends data, so it may as well use :eof => true.

How did you mock the unit test?

It uses the same mocking as many other tests in the file. It sets up the mocking and extracts the channel using setup_for_extracting_channel_action, then channel.expects(:eof!) ensures that the input stream is closed.

@carsomyr

@dylanahsmith Thanks for the explanation. How about this race condition corner case: cat -- <4gb file> with :eof => true. From my understanding, ch.eof! is called before the IO.select loop. Let's assume that at some point the OS process scheduler deactivates cat and the OS manages to completely empty the contents of its write buffer (inbound from our perspective). Wouldn't that close the SSH connection prematurely and make even the stream and capture operations unsafe?

@dylanahsmith

Let's assume that at some point the OS process scheduler deactivates cat and the OS manages to completely empty the contents of its write buffer (inbound from our perspective).

I think you misinterpreted the output buffer in the documentation for eof! as being an OS output buffer, but the relavent line only says:

The CHANNEL_EOF packet will be sent once the output buffer is empty.

eof! is implemented by just setting the instance variable @eof = true.

The process method on the channel does the following with @eof.

      if @eof and not @sent_eof and output.empty? and remote_id
        connection.send_message(Buffer.from(:byte, CHANNEL_EOF, :long, remote_id))
        @sent_eof = true
      end

The output buffer is a user-space buffer that would store the contents of a :data string. It would only send the CHANNEL_EOF packet after all the output data being sent on the channel.

@carsomyr

My concern is on the inbound (from our perspective). Let's say that the empty string has been sent and ch.eof! has been called. I am talking about the write buffer of the remote SSH process that is sending data to us.

@dylanahsmith

eof! will not close the ssh connection. It is just sending a packet to close the input stream of the command on the remote server(s). The remote process will still be able to send data. As the eof! documentation says, it just:

Tells the remote end of the channel that no more data is forthcoming from this end of the channel. The remote end may still send data.

@carsomyr

One more question: There are invoke_command invocations in the deploy recipe. Do you feel like modifying those? No biggie, since I doubt any of these care about stdin.

@dylanahsmith

invoke_command is implemented using run. I think the default behaviour of closing the input stream for commands that don't specify a block is fine. In fact, I tried to make sure existing recipes didn't need to change in order to avoid breaking third-party recipes. Let me know if you do notice regressions in existing recipes.

@carsomyr carsomyr merged commit 784c98a into capistrano:master
@carsomyr

Done, and thanks for your patience!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jul 27, 2012
  1. @dylanahsmith

    Close input streams when sending commands that don't read input.

    dylanahsmith authored
    An :eof option is added to the run method so that this can be specified
    explicitly where a block is provided that may or may not need to read from
    the input stream.
This page is out of date. Refresh to see the latest.
View
1  lib/capistrano/command.rb
@@ -221,6 +221,7 @@ def open_channels
ch.exec(command_line)
ch.send_data(options[:data]) if options[:data]
+ ch.eof! if options[:eof]
else
# just log it, don't actually raise an exception, since the
# process method will see that the status is not zero and will
View
4 lib/capistrano/configuration/actions/inspect.rb
@@ -20,7 +20,7 @@ module Inspect
# stream "tail -f #{shared_path}/log/fastcgi.crash.log"
# end
def stream(command, options={})
- invoke_command(command, options) do |ch, stream, out|
+ invoke_command(command, options.merge(:eof => true)) do |ch, stream, out|
puts out if stream == :out
warn "[err :: #{ch[:server]}] #{out}" if stream == :err
end
@@ -31,7 +31,7 @@ def stream(command, options={})
# string. The command is invoked via #invoke_command.
def capture(command, options={})
output = ""
- invoke_command(command, options.merge(:once => true)) do |ch, stream, data|
+ invoke_command(command, options.merge(:once => true, :eof => true)) do |ch, stream, data|
case stream
when :out then output << data
when :err then warn "[err :: #{ch[:server]}] #{data}"
View
5 lib/capistrano/configuration/actions/invocation.rb
@@ -138,11 +138,16 @@ def invoke_command(cmd, options={}, &block)
# and the values should be their corresponding values. The default is
# empty, but may be modified by changing the +default_environment+
# Capistrano variable.
+ # * :eof - if true, the standard input stream will be closed after sending
+ # any data specified in the :data option. If false, the input stream is
+ # left open. The default is to close the input stream only if no block is
+ # passed.
#
# Note that if you set these keys in the +default_run_options+ Capistrano
# variable, they will apply for all invocations of #run, #invoke_command,
# and #parallel.
def run(cmd, options={}, &block)
+ options = options.merge(:eof => !block_given?) if options[:eof].nil?
block ||= self.class.default_io_proc
tree = Command::Tree.new(self) { |t| t.else(cmd, &block) }
run_tree(tree, options)
View
8 test/command_test.rb
@@ -254,6 +254,14 @@ def test_process_with_unknown_placeholder_should_not_replace_placeholder
Capistrano::Command.new("echo $CAPISTRANO:OTHER$", [session])
end
+ def test_input_stream_closed_when_eof_option_is_true
+ channel = nil
+ session = setup_for_extracting_channel_action { |ch| channel = ch }
+ channel.expects(:eof!)
+ Capistrano::Command.new("cat", [session], :data => "here we go", :eof => true)
+ assert_equal({ :data => 'here we go', :eof => true }, channel[:options])
+ end
+
private
def mock_session(channel=nil)
View
4 test/configuration/actions/inspect_test.rb
@@ -12,7 +12,7 @@ def setup
end
def test_stream_should_pass_options_through_to_run
- @config.expects(:invoke_command).with("tail -f foo.log", :once => true)
+ @config.expects(:invoke_command).with("tail -f foo.log", :once => true, :eof => true)
@config.stream("tail -f foo.log", :once => true)
end
@@ -33,7 +33,7 @@ def test_stream_should_emit_stderr_via_warn
end
def test_capture_should_pass_options_merged_with_once_to_run
- @config.expects(:invoke_command).with("hostname", :foo => "bar", :once => true)
+ @config.expects(:invoke_command).with("hostname", :foo => "bar", :once => true, :eof => true)
@config.capture("hostname", :foo => "bar")
end
View
2  test/configuration/actions/invocation_test.rb
@@ -45,7 +45,7 @@ def teardown
end
def test_run_options_should_be_passed_to_execute_on_servers
- @config.expects(:execute_on_servers).with(:foo => "bar")
+ @config.expects(:execute_on_servers).with(:foo => "bar", :eof => true)
@config.run "ls", :foo => "bar"
end
View
2  test/configuration_test.rb
@@ -16,7 +16,7 @@ def test_connections_execution_loading_namespaces_roles_and_variables_modules_sh
process_args = Proc.new do |tree, session, opts|
tree.fallback.command == "echo 'hello world'" &&
session == [:session] &&
- opts == { :logger => @config.logger }
+ opts == { :logger => @config.logger, :eof => true }
end
Capistrano::Command.expects(:process).with(&process_args)
Something went wrong with that request. Please try again.