Use max_select_wait_time to limit IO#select sleep

[eugeneius]

Followup to #8.

If keepalive is enabled but no interval is provided, we should use the same default interval as Net::SSH. Currently the keepalive setting is ignored unless an interval is provided.

[mfazekas]

i'd rather add somthing to net-ssh, there seems to be too much logic duplication. Somthing like this:

servers.map { |s| s.io_select_wait }.compact.min

[eugeneius]

If we make the keepalive attribute on Net::SSH::Connection::Session public, we could do this:

class Net::SSH::Multi::Session
  def keepalive_interval
    server_list.map { |s| s.keepalive_interval }.compact.min
  end
end

class Net::SSH::Multi::Server
  def keepalive_interval
    session.keepalive.interval if session && session.keepalive.enabled?
  end
end

Alternatively we could check enabled? in Net::SSH, and expose a keepalive_interval method:

class Net::SSH::Connection::Session
  def keepalive_interval
    @keepalive.interval if @keepalive.enabled?
  end
end

The problem is writing tests; right now the Net::SSH::Multi test suite stubs out all interaction with Net::SSH, but to test the implementation described above the Server object has to have an active Session, which means calling Net::SSH.start, which will try to open a socket...

[mfazekas]

I don't think your proposal solves DEFAULT_IO_SELECT_TIMEOUT . I don't think that net-ssh-multi should be concerned with keepalive. It should ask the ssh session of how soon it need to be run pre/postprocess callbacks. So i think something like session.max_select_wait_time would work better.
Then max_selcect_wait = server_list.map(&:max_select_wait_time).min

[eugeneius]

My proposal intended to solve DEFAULT_IO_SELECT_TIMEOUT by going through this method.

server_list is a collection of Net::SSH::Multi::Server objects. If we want to avoid duplicating logic from Net::SSH, max_select_wait_time will have to talk to its session attribute, which is a Net::SSH::Connection::Session. Without a way to fake a session in this gem's test suite, it will be difficult to write tests.

Sorry for the back and forth here, but I'm not comfortable adding an interface between the two gems without being able to test it 😕

[mfazekas]

I see, i thought that server list is a list of net-ssh sessions, otherwise the current options will not contains the keepalive options read by net-ssh via config files.

so this would be my modified suggestion.

server_list.map(&:max_select_wait_time).compact.min
...

#in Net::SSH::Multi::Server:
def max_select_wait_time
  session.max_select_wait_time if session
end

As for testing we're already using mocha, so we should be able to do what we'd like.

Like

s1 = @session.use('h1')
s1.expects(:max_select_wait).returns(20)

I do prefer max_select_wait_time vs keepalive as that means the two projects are more decoupled so we can change/reorg the keepalive in net-ssh shall we need to.

[eugeneius]

You're right, we need to go through Net::SSH to pick up the config file options.

I agree on the name too - I'll submit a pull request to add it shortly. Thanks for the explanation!

[eugeneius]

I've added the method to Net::SSH in net-ssh/net-ssh#343, and updated this pull request to use it.

We'll have to bump the minimum Net::SSH requirement here once a new version is released.

[eugeneius]

@mfazekas I see Net::SSH v4.0.0 was finally released with net-ssh/net-ssh#343 included 🎉

Can we bump the version requirement in Net::SSH::Multi and finally ship this?