Make SSH_TIMEOUT an overridable argument

[thatmattlove]

Hello,

I'm working to integrate this awesome project into a project of mine, but kept running into an issue when testing connectivity to an SSH proxy server that was unreachable. In my tests, the tunnel connection took a full 120 seconds to time out and raise an exception. When I modified the original constant SSH_TIMEOUT to, say, 10, the session timed out after 10 seconds as was useful for my purposes.

In this PR I set a generous default timeout period of 30 seconds, but this could be adjusted if needed.

Thanks!

[coveralls]

Coverage remained the same at 91.948% when pulling 6a363af on checktheroads:master into 397ee78 on pahaz:master.

[pahaz]

This change is not backward compatible. It looks nice, but what the reason?

[thatmattlove]

Hi @pahaz - sorry for the delay in response. The reason is that the default timeout period is too long to detect an inactive gateway. In my project, if a session is initiated to a device that is unreachable or otherwise inaccessible, the default 120s timeout must expire prior to any further action taking place which is a problem in my case. Can you provide more detail on how this change is not backwards compatible? It seems to me that the codebase relies on calling self.gateway_timeout in the class instance, which is currently card coded. I'd be happy to make other changes needed to make this backwards-compatible, I'm just not sure what would need to change.

Thanks!

[nickodell]

@checktheroads It seems to me that if you made the default None, then that would be backwards compatible with the current behavior. (i.e. if someone has a server which takes 31 seconds to respond, their code will keep working when they upgrade to the new version.)

This does mean that people would only benefit from having a timeout if they explicitly set one. In general, I do think it is a good idea to have timeouts by default, but I think it's also reasonable to insist on backwards compatibility.

[thatmattlove]

@nickodell thank you - wasn't following at first, but your mention of None made me look at the commit and see that I did not set the default to None, which is what I thought I did. Fixed in 71c80871.

@pahaz - will this suffice?

PS: something appears to be wrong with CircleCI, I see the tests are failing on this commit due to ImportError at from coverage.report import Reporter in the test process.

[nickodell]

PS: something appears to be wrong with CircleCI, I see the tests are failing on this commit due to ImportError at from coverage.report import Reporter in the test process.

It does pass tests in Python 3.4, though. 🎉

It looks like the test error is related to this issue: TheKevJames/coveralls-python#213

sshtunnel depends on tox, which depends on the coverage library. Coveralls also depends on coverage library, so the first time you install dependencies, you install a version which is incompatible with coveralls.

We should try one of the recommendations in that thread:

My best guess would be this is related to the recent coverage==5.0 release. We have an issue to fix this (TheKevJames/coveralls-python#203) but I've been traveling a few weeks now and haven't had a chance to fix things up.

This coveralls library has a pin to coverage<5, but if you're installing coverage separately, you may be sidestepping this version pin -- could you try doing a pip install coverage<5?

[pahaz]

I know that someone uses code like so:

import sshtunnel
import threading
import time

# incompatible !
sshtunnel.SSH_TIMEOUT = 3

tunnel = sshtunnel.SSHTunnelForwarder(
            ('172.18.19.20', 22),
            ssh_username='user',
            ssh_password='password',
            remote_bind_address=('192.168.1.10', 24),
            set_keepalive=0.5
        )

It's already a part of a public API :(

You can just use it as default for gateway_timeout

[nickodell]

@pahaz

I hadn't considered that. Okay, that's fixable.

What do you think about the following?

1. Leave SSH_TIMEOUT where it is.

#: Timeout (seconds) for the connection to the SSH gateway, ``None`` to disable
SSH_TIMEOUT = None

2. In the constructor for SSHTunnelForwarder, set self.gateway_timeout based upon both the argument and the global value:

self.gateway_timeout = gateway_timeout if gateway_timeout is not None else SSH_TIMEOUT

In the public API, there would be two ways to set the timeouts. One would be global, and the other would be for a single instance. If you set both timeouts, the setting on the specific instance is used.

[pahaz]

it's ok

[pahaz]

Please update from master. It looks like a conflict with https://github.com/pahaz/sshtunnel/pull/195/files

[eladavron]

Just my additional 2 cents: I encountered an issue with my project where the gateway was stuck on trying to connect to a downed host for a couple of days and had to be terminated manually, where introducing the exact change this PR is suggesting (and overriding the timeout to something sensible) solved it.

[ignacioparicio]

Same problem in my project. Using sshtunnel==0.4.0, SSH_TIMEOUT does not seem to be configurable