Fix open two connections with changed cache key

[shirosaki]

args.to_s is used for cache key of connection pooling.
ssh_options in args is changed while creating connections in
connection_factory.call(*args).
Resetting cache key using changed args prevents cache miss.

This is a workaround for #369.

[leehambley]

Hi @shirosaki thanks for the patch!

I'm really glad to see tests, and that you solved a bug. I'm not a 100% happy with the implementation copying changes into the cache keys.. maybe we could find another way to derive cache keys.

I'd be happy to merge this as-is (it has a test for correct behaviour) and eventually one-day re-evaluate (maybe) how we derive cache keys.

In short LGTM, I'm happy to merge if one of the other maintainers is!

[mattbrictson]

Thanks for the PR!

I definitely think you are onto something here: the idea of re-computing the key after opening the connection sounds like a workaround I would be happy with.

However I have some concerns with the current implementation:

1. I believe the bug is due to option values changing, not the keys. No new keys are added, and no keys are mutated. The issue is that one of the values is mutated. The fix (and the corresponding test) should be narrowly focused on this scenario.
2. If we simply add the cache with a recomputed key (i.e. without removing the original key) then we will have two references to the same cache in the caches hash. We should avoid this if possible. There are places in the code where we iterate over the connections with caches.values, and duplicates could lead to bugs.

Is there a way you could rework this PR to address these points?

Also, some nitpicks:

• I think args.last would be preferable to the somewhat arbitrary args[2].
• For code readability, I would like the "update cache key with changed args" logic to be factored into its own method with a self-documenting method name.

[shirosaki]

Thanks for feedback.

The bug is due to option values changing and the cache key of args.to_s changing.
It would be nice if we could find new way to derive cache key.

I updated the patch for the following points.

• delete old key instead of just adding new key
• use args.last
• refactoring to a separated method

[mattbrictson]

Thanks for the changes. I need some more time to review and think this over. It looks to me like this could be simplified even further, but I haven't figured it out yet.

[mattbrictson]

I think there are a couple things I would change about this PR.

First, the test is not really an accurate reproduction of the original bug (#369). In the bug, the same SSH options hash is passed both times, and the hash contains the same keys both times. It is only the value of one the keys (:known_hosts) that gets mutated.

If we change the test to specifically cover only this scenario, then the code changes needed to make that test pass become a bit more simple: all we need to do is compare the key (i.e. args.to_s) that was used prior to connection_factory.call with the key that is generated after that call. If the key has changed (i.e. due to the options hash being mutated), then we update the cache.

Since this re-keying only needs to happen for the Cache and not for NilCache (i.e. when the connection pool is disabled), it makes sense to put the "key changed" logic on the cache object itself.

Anyway, I have pushed a commit to your branch with these proposed changes. Let me know what you think!

[shirosaki]

Thanks for the commit. Indeed simplified.
But unfortunately the patch didn't solve password twice issue for my project using capistrano.

I did some fixes to avoid password twice.
shirosaki@43d6aa2

Host#netssh_options creates new Hash from ssh_options when calling with. So new Hash would be required.
https://github.com/capistrano/sshkit/blob/master/lib/sshkit/host.rb#L66

:password_prompt and :logger options are added in net-ssh Net::SSH.assign_defaults.
The key in Cache object should be updated for comparing next time.

[mattbrictson]

Yes, you are right. This bug goes much deeper than I originally realized. Thanks for pointing out that :password_prompt and :logger are being added by net-ssh; I didn't even know that was happening.

Maybe it is just me, it is difficult to follow what exactly is happening in the code, and as I've discovered it takes a lot of digging to understand why the workaround is written the way it is. I would therefore like to hold off on merging this PR.

Perhaps there is a better way to solve this? What if we opened a PR at net-ssh to change how they handle default options such that they make a copy of the options hash instead of modifying it in place? Would that make our workaround easier?

[shirosaki]

I found another fix.
5a80b0d
Set default options early in SSHKit::Backend::Netssh.
It is the same timing as :known_hosts default.

[mattbrictson]

Very promising! This looks great. I'll review in more detail tomorrow. Thanks

[mattbrictson]

Is the chained assignment really needed here? I find it a bit hard to read.

If I remove the extra assignment and just write:

caches[new_key] = caches.delete(cache.key)
cache.key = new_key

… then the tests still pass.

[shirosaki]

You are right. I'll fix it.

[mattbrictson]

Awesome! ✨

I think this is good to merge, provided you can:

1. Rebase on master and fix the conflicts
2. Review my code comment above

Thanks for all your hard work on fixing this bug!

[shirosaki]

Thanks for review.

• rebase on master and fix conflicts in CHANGELOG.md
• remove unneeded assignment

[mattbrictson]

There is one functional test that is failing:

SSHKit::Backend::TestNetssh
  test_ssh_option_merge                                           FAIL (3.55s)
Minitest::Assertion:         --- expected
        +++ actual
        @@ -1 +1 @@
        -[:forward_agent, :known_hosts, :paranoid]
        +[:forward_agent, :known_hosts, :logger, :paranoid, :password_prompt]
        /Users/mbrictson/Code/sshkit/test/functional/backends/test_netssh.rb:62:in `test_ssh_option_merge'

If you are unable to run the functional tests locally, I can take care of it. Let me know!

[shirosaki]

Fixed functional test.

BTW functional test causes many errors on Windows.

[mattbrictson]

BTW functional test causes many errors on Windows

Thanks for pointing that out. The functional tests don't work on Travis either, so there is definitely a lot of room for improvement!

[mattbrictson]

Tests all pass. Thank you so much for this PR and all your research! 🙇