Skip to content

Add a timeout to the url requests #207

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

Merged
merged 4 commits into from
Feb 6, 2023
Merged

Add a timeout to the url requests #207

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
58397f0
Add a timout to the url requests
Jaimepas77 Jan 25, 2023
6fc05bc
Update tldr.py
Jaimepas77 Feb 5, 2023
e377115
Fix timeout exception
Jaimepas77 Feb 5, 2023
01b8f64
Increase timeout to 10 seconds
MasterOdin Feb 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions tldr.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,7 @@ def get_page_for_platform(
try:
data = urlopen(
Request(page_url, headers=REQUEST_HEADERS),
timeout = 2,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
timeout = 2,
timeout=2,

Copy link
Member

Choose a reason for hiding this comment

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

Is two seconds long enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is two seconds long enough?

I think so, but i wouldn't care if it was set to 10 seconds or something like that. The problem that i saw was that, if no timeout was specified, there were some occasions where the program would get stuck for more than a minute until throwing a timeout error. However I believe that 2 seconds are more than enough.

Copy link
Collaborator

@MasterOdin MasterOdin Feb 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I've bumped this up to 10 seconds as I feel like while 2 is probably fine, better to be a touch conservative and hopefully avoid any people complaining about things "suddenly" breaking on them due to timeouts, and this still at least more rapidly gets this program to crash out much more quickly.

context=URLOPEN_CONTEXT
).read()
data_downloaded = True
Expand Down Expand Up @@ -267,6 +268,8 @@ def get_page(
except URLError:
if not PAGES_SOURCE_LOCATION.startswith('file://'):
raise
except TimeoutError:
continue
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure we should be skipping this, in that we don't skip if we get a 500 HTTPError. Something about the upstream is broken in a way that we cannot connect to it, and I don't think we should continue on and potentially serve the wrong page.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've read some more info about that and I've noticed that in fact a URLError (and not a TimeoutError) is raised when the timeout expires. Consequently I've deleted that part of the code and committed it. The behavior is that whenever a request is not answered an error message is shown to the user. Sorry for the confusion.


return False

Expand Down