Skip to content

newtorrents: restore global timeout setting#2323

Closed
radfish wants to merge 1 commit intoFlexget:developfrom
radfish:PR--newtorrents-timeout-scope
Closed

newtorrents: restore global timeout setting#2323
radfish wants to merge 1 commit intoFlexget:developfrom
radfish:PR--newtorrents-timeout-scope

Conversation

@radfish
Copy link
Copy Markdown
Contributor

@radfish radfish commented Feb 3, 2019

Motivation for changes:

The newtorrents plugin sets the socket timeout globally for all plugins by configuring it at the lowest socket layer, even when the newtorrents plugin was not even used by the yml config. This broke other plugins, for which a 10 second timeout was inappropriate.

Detailed changes:

  • Save and restore timeout for every network operation using the 'with' pattern

Addressed issues:

  • No issue filed, but it's a bug fix.

To Do:

Unfortunately, I do not use newtorrents and I don't see a test for it, so this is only tested to the extent that Flexget executes with this patch, so the newtorrents.py file is parsed successfully.

The way it was set the timeout for all plugins, even when
the newtorrents plugin was not used. This broke other plugins, for
which a 10 second timeout was inappropriate.
@gazpachoking
Copy link
Copy Markdown
Member

Hmm, could we just switch it to using the timeout parameter to requests? i.e. requests.get(url, timeout=10)

@radfish
Copy link
Copy Markdown
Contributor Author

radfish commented Feb 5, 2019 via email

@gazpachoking
Copy link
Copy Markdown
Member

gazpachoking commented Feb 5, 2019

That timeout was added 11 years ago, before we were even using requests (b23a604.) I say let's nuke it.

We also have a default timeout of 30 seconds on our requests session (

def __init__(self, timeout=30, max_retries=1, *args, **kwargs):
,) so I don't even think we need to add an explicit timeout parameter to the requests calls.

@gazpachoking
Copy link
Copy Markdown
Member

Thanks for this @radfish. I went ahead and just ripped out the socket timeout stuff.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants