Skip to content
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

Fixes #2045 Timeouts should be decided with a monotonic clock #2046

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

ibygrave
Copy link

@ibygrave ibygrave commented May 7, 2022

I've tried to retain Python 2.7 compatibility by using timeit.default_timer as the monotonic clock. If Python 2.7 support was not needed this change would be simpler.

Ian Bygrave added 2 commits May 7, 2022 22:39
This is a monotonic clock suitable for measuring durations and timeouts.
@bskinn
Copy link
Contributor

bskinn commented May 8, 2022

Python 2 support is being removed in the relatively near term, with a 3.0 release. Given that this isn't an urgent fix, I'd say go ahead and rewrite in the simpler, Python 3-only fashion, and we'll just aim it for 3.0.

Keep the current git history intact, though, so that if @bitprophet should want to use the Python 2-compatible version he can just cherry-pick from 94d840f or thereabouts.

@ibygrave
Copy link
Author

ibygrave commented May 8, 2022

Thanks.

For the Python 3 only version would you prefer I keep the paramiko.common.timer abstraction, or use time.monotonic directly? I'll remove the abstraction unless you have a preference.

@bskinn
Copy link
Contributor

bskinn commented May 8, 2022

That's a call for @bitprophet -- I'll bump the question to his attention.

@bitprophet
Copy link
Member

Re: Python 2, nah, that's going out the window RSN.

Re: whether it's nice hiding time.monotonic behind paramiko.common.timer, that's still a good question. My gut says that the way it's phrased right now - exposing the use of monotonic - is better for two reasons:

  • Just less code/indirection. Always good.
  • Makes it slightly more obvious to readers/contributors that "you should be using monotonic for timeout related code". I wasn't even aware that monotonic existed until reading this, though it makes a lot of sense (avoids timeouts from incorrectly firing during time changes like DST, NTP/PTP updates, etc).
    • So having this sprinkled about might clue somebody else in that "oh I should reach for this, not time.time, when I care about elapsed durations".

I'll put this in a milestone so there's a chance I'll get to it when crafting 3.0. Thanks!

@bitprophet bitprophet added this to the 3.0 milestone Jun 3, 2022
@bitprophet
Copy link
Member

Linking to #2045 (doing it in the subject doesn't work, sadly)

@bskinn bskinn added the Feature label May 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants