New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pass a timeout value when requesting a new channel to the server #1475

Closed
EugeniuZ opened this Issue Jun 17, 2016 · 11 comments

Comments

Projects
None yet
3 participants
@EugeniuZ

EugeniuZ commented Jun 17, 2016

Hi,

Here: https://github.com/fabric/fabric/blob/master/fabric/state.py#L409 there is a no-arg call to open_session()
and here: https://github.com/paramiko/paramiko/blob/master/paramiko/transport.py#L677 open_session() supports a keyword timeout parameter.

open_session() calls open_channel() which in turn would wait for up to 1h before raising a connection error,
see here: https://github.com/paramiko/paramiko/blob/master/paramiko/transport.py#L788
and here: https://github.com/paramiko/paramiko/blob/master/paramiko/transport.py#L828

Would it be possible to pass env.timeout or env.command_timeout in the call to open_session() to make the wait time configurable ?

@bitprophet

This comment has been minimized.

Member

bitprophet commented Jun 17, 2016

This sounds like a reasonable request to me (including for 1.x, it would be small enough to not affect stability), I don't have time to implement myself but would definitely consider a patch from somebody. Thanks for the report!

@jrmsgit

This comment has been minimized.

jrmsgit commented Jul 22, 2016

Just created PR #1490, maybe that works? I used env.timeout instead of env.command_timeout as the first one seems to be network related.

Maybe another solution could be something like: t = env.timeout if (env.command_timeout is None) else env.command_timeout

@bitprophet

This comment has been minimized.

Member

bitprophet commented Jul 25, 2016

@jrmsgit thanks! keeping discussion in here for cleanliness...

  • Agree on reuse of env.timeout as this is a network-related timeout; env.command_timeout is pretty distinct, I don't think anyone would assume it'd be reused for this.
  • An issue I only noticed while looking at this: that timeout parameter on Paramiko's end was only added recently, specifically as part of paramiko/paramiko#491 - so we'd need to also bump Fabric's minimum Paramiko version if we do this. Torn on that.
    • Right now that min version is 1.10, so bumping to the below is a decent sized jump.
      • Not the end of the world but not something to take lightly, especially pending folks using distro-packaged versions, and the fact that it takes us past 1.13 (which hurt stability somewhat - tho by 1.15/1.16 it's definitely better).
      • Probably wise for me or someone to audit that and keep a list somewhere, honestly. What versions of Paramiko and PyCrypto do various distros have packaged up?
    • Annoyingly, I screwed up when approving that Paramiko PR, it went into a bugfix release instead of a feature one, despite hosting an API change. So it's hard for us to nail down a one-size-fits-all paramiko >=XXX change for Fabric's setup.py.
      • Offhand it'd have to be either >=1.15.3, or >=1.16 if we wanted to be "clean" about it.
      • I don't think setuptools or pip let us have a multivariant version specifier, and saying >=1.13.4 won't work as e.g. 1.14.0 satisfies that but lacks the necessary API.
    • Strongly related to this is #1461
@jrmsgit

This comment has been minimized.

jrmsgit commented Jul 27, 2016

@bitprophet thanks for looking into it...

I understand what you mean... What about wrapping then the call to open_session in a try/except block?

Something like:

try:
    open_session(timeout=t)
except TypeError:
    # old paramiko
    open_session()
@bitprophet

This comment has been minimized.

Member

bitprophet commented Jul 28, 2016

Might work, though users' expectations that the timeout setting is being honored, would still be violated (for those that are aware it exists). A warning message could hedge against that, but for something so frequently executed it would be noisy.


Maybe the right approach is to make it a new setting entirely (env.connection_timeout?), defaulting to None (or some "is not set" sentinel value), and then only pass it if it has been set to non-None/sentinel.

This has the property of ensuring a user is aware they've taken advantage of a specific feature, and then if it explodes/warns, they'd be more likely to realize what's up.

It would also be backwards compatible behavior wise...reusing env.timeout would technically change the behavior for anyone who is currently used to / expecting / desiring slow, >10s connection time. (That is probably not many people but I bet it's nonzero).


I'm also not 100% opposed to sucking it up and bumping the paramiko dependency at some point; there've been a very significant number of changes, including security fixes, in Paramiko since the 1.10 days. I don't love the idea but it's an option.

@jrmsgit

This comment has been minimized.

jrmsgit commented Jul 29, 2016

Right, I see... I guess that the "warning message" approach could work, but maybe in a "only alert once" way, but I'm not sure if that's possible... (avoid dup messages).

The new env.arg could work too I think, but having 3 different timeout settings could be annoying / confusing probably too...

Not sure, to be honest...

@bitprophet

This comment has been minimized.

Member

bitprophet commented Jul 30, 2016

Generally, having granular settings is a good thing (IMO), there are always users that need the granularity. If the defaults can align (i.e. if in a fresh codebase / new release, connection timeout simply inherited regular timeout's value by default) it means the less-advanced users don't have to know about the 'expert' settings until they need them.

In older codebases where backwards compat is a concern, yea, it gets slightly more annoying. But that's another reason to put out X.0 releases more often than "a decade". (innocent whistling...)

@jrmsgit

This comment has been minimized.

jrmsgit commented Aug 1, 2016

Agree on that having granular settings is a good thing, but it's IMHO too... Not sure about the rest of the world :)

If you think it worth, I could work on my original PR to make it use a new env.connection_timeout setting (or name it as your prefer...)

@bitprophet

This comment has been minimized.

Member

bitprophet commented Aug 24, 2017

Revisiting this now (itch scratch). Not sure why it didn't occur to me before but another solution to the "this requires newer Paramiko" is to just conditionally pass it if paramiko supports it, probably just in EAFP style (try/except.)

Not amazing but probably least-bad.

FTR Paramiko must be 1.14.3 or higher to allow this param (grump @ past self, that should have been a 1.15 addition.)

@bitprophet

This comment has been minimized.

Member

bitprophet commented Aug 25, 2017

Confirmed arity error on Paramiko 1.14.2 and success on 1.14.3; confirmed try/except works fine for that for now.

bitprophet added a commit that referenced this issue Aug 25, 2017

@bitprophet

This comment has been minimized.

Member

bitprophet commented Aug 25, 2017

This is out as Fabric 1.14.0 on PyPI.

@bitprophet bitprophet closed this Aug 25, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment