Skip to content

updater.py: Better handling of timeouts during getUpdates - #1007

Merged
tsnoam merged 2 commits into
masterfrom
fix_802
Feb 18, 2018
Merged

updater.py: Better handling of timeouts during getUpdates#1007
tsnoam merged 2 commits into
masterfrom
fix_802

Conversation

@tsnoam

@tsnoam tsnoam commented Feb 15, 2018

Copy link
Copy Markdown
Member

TimedOut exception is an expected an normal event. To reduce noise and
make things more "fluent" we now:

  • Make sure that we don't sleep after the timeout but rather retry
    immediately.
  • Log debug instead of error level.

Fixes #802

TimedOut exception is an expected an normal event. To reduce noise and
make things more "fluent" we now:
 - Make sure that we don't sleep after the timeout but rather retry
immediately.
 - Log debug instead of error level.

Fixes #802
@tsnoam
tsnoam requested a review from Eldinnie February 15, 2018 16:13

@Eldinnie Eldinnie left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally it looks good. But I'm wondering. This will probably work fine for a shortly interrupted connection, but won't this hog memory when timedout is fired often?

Comment thread telegram/ext/updater.py Outdated
cur_interval = 0.5 + e.retry_after
except TimedOut as toe:
self.logger.debug('Timed out getting Updates: %s', toe)
# If getUpdates() failed due to timeout, we should retry asap.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get_updates() maybe?

@tsnoam

tsnoam commented Feb 17, 2018

Copy link
Copy Markdown
Member Author

@Eldinnie
There will be no memory hogging. If you've meant CPU hogging, I don't see how it's possible as if the next get_updates() will be interrupted by:

  1. A detectable network error, then the except TelegramError as te: block will come into affect.
  2. A timeout - the wait itself for the timeout to occur will take enough time to avoid any CPU hogging.

@Eldinnie

Copy link
Copy Markdown
Member

Good, merge after #1006

@tsnoam

tsnoam commented Feb 17, 2018

Copy link
Copy Markdown
Member Author

@Eldinnie Why after?

@Eldinnie

Copy link
Copy Markdown
Member

@tsnoam not really needed maybe, but that will make CI pass. And I prefer t if master passes CI

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TimedOut on get_updates

2 participants