Fix Disabling of Connection Pool

[tisba]

Fixes#438

The README currently states, that:

SSHKit::Backend::Netssh.pool.idle_timeout=0

will disable the cache and this can be used to mitigate potential problems from connection pooling. As described in #438 quite the opposite was the case. Connections were not cached, BUT the cache eviction loop was still started. With idle_timeout = 0 the the eviction loop becomes a busy loop hogging one core at 100%.

This PR will skip the launch of the eviction loop (via run_eviction_loop) in case the cache is disabled. If the cache is disabled, there can't be any timed_out_connections because silently_close_connection_later is not used.

find_cache does not use thread_safe_find_or_create_cache if cache is disabled:

sshkit/lib/sshkit/backends/connection_pool.rb

Lines 98 to 106 in ac18160

	# Look up a Cache that matches the given connection arguments.
	deffind_cache(args)
	ifcache_enabled?
	key=cache_key_for_connection_args(args)
	caches[key] || thread_safe_find_or_create_cache(key)
	else
	NilCache.new(method(:silently_close_connection))
	end
	end

I'd like to get some guidance on how to test this (@leehambley maybe?).

Further Suggestion

I also would like to propose that there is a maximum eviction frequency. The idle_timeout should not guarantee when idle connections are removed. Rather it should be considered "after idle_timeout sshkit will evict the connection eventually; never before". If you agree with this, we could simplify the sleep from this:

sshkit/lib/sshkit/backends/connection_pool.rb

Lines 129 to 139 in ac18160

	# Loops indefinitely to close connections and to find abandoned connections
	# that need to be closed.
	defrun_eviction_loop
	loopdo
	process_deferred_close
	# Periodically sweep all Caches to evict stale connections
	sleep([idle_timeout,5].min)
	caches.values.each(&:evict)
	end
	end

to

sleep5

[capistrano-bot]

	1 Warning
⚠️	There are code changes, but no corresponding tests. Please include tests if this PR introduces any modifications in behavior.

Generated by 🚫 Danger

[leehambley]

@tisba thanks for the submission, this seems like a great fix. It's dirt simple, and the explanation in the PR is longer than the code sample, which inspires confidence.

I've requested a review from @mattbrictson if he's OK with it, let's roll it and get it out in the next release!

[tisba]

Nice!

Do you want to me to update the changelog, as the nice @capistrano-bot mentioned? :)

Also, what about the other suggestion regarding the slight adjustment of the eviction loop as well?

[leehambley]

Do you want to me to update the changelog, as the nice @capistrano-bot mentioned? :)

Probably a good idea!

Also, what about the other suggestion regarding the slight adjustment of the eviction loop as well?

That concerns me somewhat more than this, lets keep them separate? @mattbrictson what do you think?

[mattbrictson]

Also, what about the other suggestion regarding the slight adjustment of the eviction loop as well?

I think it is safe to include. Just to clarify, we're talking about changing this line:

sleep([idle_timeout,5].min)

to be:

sleep(5)

correct?

There are code changes, but no corresponding tests. Please include tests if this PR introduces any modifications in behavior.

I am OK with accepting the PR without tests.

Thanks so much for this PR! 🙏

[tisba]

This is exactly the change I would like to suggest.

With 162cbfd the eviction of timed out connection could obviously take a couple of seconds, but (like I already outlined) the semantics of "will eventually be closed/removed" is not something that users of sshkit should be surprised by.

[leehambley]

Danke für den Einsatz!

Thanks for your contribution, may your SideKiq workers spin less frequently on a busy loop! 🏆

[mattbrictson]

We generally publish new SSHKit releases to rubygems every six weeks, and since the most recent release was just a few days ago, that would mean this fix doesn't get published until about 5 weeks from now. But I'm happy to make exceptions for important bug fixes. Does this qualify?

[tisba]

IMO this is a nasty bug, even though sshkit users might run into it only extreme rare cases. What makes it "worse" IMO, is that the current README (regarding the ability to disable the connection cache) directly leads to the issue fixed by this PR.

On the other hand the mitigation (once you've figured out what is going on), is rather trivial: Don't disable the connection pool.

Me personally I'm okay with not rushing this.

[mattbrictson]

Actually, I'll make a release today. These sorts of major bug fixes don't happen very often.

[mattbrictson]

🚀 released as 1.18.2