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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 2 commits into from
Feb 18, 2018
Merged

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

merged 2 commits into from
Feb 18, 2018

Conversation

tsnoam
Copy link
Member

@tsnoam tsnoam commented Feb 15, 2018

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
updater.py: Better handling of timeouts during getUpdates
34c1e08 
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 reviewed Feb 15, 2018
View reviewed changes
Copy link
Member

@Eldinnie Eldinnie left a comment

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?

telegram/ext/updater.py Outdated
@@ -288,8 +286,12 @@ def _start_polling(self, poll_interval, timeout, read_latency, bootstrap_retries
except RetryAfter as e:
self.logger.info(str(e))
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
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
Copy link
Member Author

tsnoam commented Feb 17, 2018

@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
Member

Good, merge after #1006

@tsnoam
Copy link
Member Author

tsnoam commented Feb 17, 2018

@Eldinnie Why after?

@Eldinnie
Copy link
Member

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

@tsnoam tsnoam mentioned this pull request Feb 18, 2018
Closed
@tsnoam tsnoam merged commit 91ae35e into master Feb 18, 2018
@tsnoam tsnoam deleted the fix_802 branch February 18, 2018 15:50
This was referenced Mar 2, 2018
Closed
Closed
Merged
Merged
Merged
@github-actions github-actions bot locked and limited conversation to collaborators Aug 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Eldinnie Eldinnie Eldinnie left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

TimedOut on get_updates
2 participants
@tsnoam @Eldinnie