When ServiceUnavailable, log error instead of exception (traceback) #764

fernandog · 2017-06-19T11:34:12Z

@Diaoul imho 503 status is not an unexpected error (exception). It's quite common btw. This PR avoids showing a traceback to the user and its show an error log message instead Provider {provider} unavailable, discarding it

Also OpenSubtitle raises a xmlrpclib.ProtocolError when unavailable

Full traceback is in the commit message.
What do you think?

Diaoul · 2017-06-20T08:46:24Z

subliminal/core.py

+    elif r.status_code == 503:
+        raise ServiceUnavailable
+    else:
+        r.raise_for_status()


I agree this is not ideal to have unavailability errors go straight up as unhandled exceptions.

r.status_code == 503 already raises HTTPError with response attribute on which you can check the status code

try: r.raise_for_status() except Exception as e: # e.reponse.status_code == 503

TooManyRequests is quite provider specific, I'm not sure this is relevant to put this in a global function.
Sometimes raise_for_status isn't used because one expects a redirect or something like that.

I suggest a less invasive approach: catch HTTPError with 503 status code pretty much as ServiceUnavailable and leave the TooManyRequests handling on a provider basis.
What do you think?

@Diaoul thanks for reviewing.
I added TooManyRequests only because of Addi7ed. I will add a custom to the provider then

Just a fyi, I did all this based on OpenSubtitles:
https://github.com/Diaoul/subliminal/blob/develop/subliminal/providers/opensubtitles.py#L268-L294

should we change that method too as it checks 503 and others status codes?

OpenSubtitles does that because there is no raise_for_status available. I agree that ServiceUnavailable shouldn't be related to OpenSubtitles though but maybe a broader exception class within whole subliminal for providers that don't use requests and the HTTPError exception that is.

fernandog · 2017-06-20T12:43:32Z

subliminal/core.py

+            if hasattr(e, 'response') and getattr(e.response, 'status_code', None) == 503:
+                logger.error('Provider %r terminated unexpectedly', name)
+            else:
+                logger.exception('Provider %r terminated unexpectedly', name)


@Diaoul like this?

Better to catch HTTPError from requests no?

fernandog · 2017-06-20T16:33:51Z

subliminal/core.py

+            if e.response and e.response.status_code == 503:
+                logger.error('Provider %r unavailable, improperly terminated', name)
+            else:
+                logger.exception('Provider %r http error, terminated unexpectedly', name)


@Diaoul all good?

fernandog · 2017-06-21T17:54:38Z

@Diaoul can we make TooManyRequests from Addi7ed a "expected error" and also log as unavailable (or else) instead of log exception? It raises that error a lot in my tests with this PR

ERROR:subliminal.core:Unexpected error in provider 'addic7ed'
Traceback (most recent call last):
  File "build/bdist.linux-x86_64/egg/subliminal/core.py", line 118, in list_subtitles_provider
    return self[provider].list_subtitles(video, provider_languages)
  File "build/bdist.linux-x86_64/egg/subliminal/providers/addic7ed.py", line 273, in list_subtitles
    return [s for s in self.query(video.series, video.season, video.year)
  File "build/bdist.linux-x86_64/egg/subliminal/providers/addic7ed.py", line 238, in query
    raise TooManyRequests()

ping @pannal

as per you comment in 304, maybe we can work this out here in this PR. what do you think?

pannal · 2017-06-23T20:29:50Z

Sure. I think we definitely need a more fine grained provider status handling than what's currently implemented. Is 304 on addic7ed really their "too many requests" response?

@fernandog you can join us on slack btw: https://szslack.fragstore.net

fernandog · 2017-06-23T20:50:21Z

@Diaoul @pannal

this are my findings using a modified pytest

Provider return a 304 and content = '' so that's why we have the indexError

can we assume site is not available?

self = <Addic7edProvider [(<class 'subliminal.video.Episode'>,)]>
series = 'The Big Bang Theory', season = 7, year = 2007, country = None

    def query(self, series, season, year=None, country=None):
        # get the show id
        show_id = self.get_show_id(series, year, country)
        if show_id is None:
            logger.error('No show id found for %r (%r)', series, {'year': year, 'country': country})
            return []
    
        # get the page of the season of the show
        logger.info('Getting the page of show id %d, season %d', show_id, season)
        r = self.session.get(self.server_url + 'show/%d' % show_id, params={'season': season}, timeout=10)
        print r
        raise_for_status(r)
        soup = ParserBeautifulSoup(r.content, ['lxml', 'html.parser'])
    
        # loop over subtitle rows
>       match = series_year_re.match(soup.select('#header font')[0].text.strip()[:-10])
E       IndexError: list index out of range

..\subliminal\providers\addic7ed.py:239: IndexError
---------------------------- Captured stdout call -----------------------------
<Response [304]>
---------------------------- Captured stderr call -----------------------------
No handlers could be found for logger "bs4.dammit"
========================== 1 failed in 59.79 seconds ==========================
Process finished with exit code 0

coveralls · 2017-07-21T18:46:32Z

Coverage decreased (-1.0%) to 90.917% when pulling c483df4 on fernandog:catch_unavailable into 461d33c on Diaoul:develop.

coveralls · 2017-07-28T14:04:48Z

Coverage decreased (-0.3%) to 91.597% when pulling 69bcb87 on fernandog:catch_unavailable into 4062ba5 on Diaoul:develop.

coveralls · 2017-07-28T14:04:49Z

Coverage decreased (-0.3%) to 91.597% when pulling 69bcb87 on fernandog:catch_unavailable into 4062ba5 on Diaoul:develop.

coveralls · 2017-07-28T14:33:37Z

Coverage decreased (-0.3%) to 91.597% when pulling 69bcb87 on fernandog:catch_unavailable into 4062ba5 on Diaoul:develop.

fernandog · 2017-07-28T14:43:04Z

Travis failing only for Addi7ed using lxml parser (known issue)

503 status is not an unexpected error. Quite common btw Also OpenSubtitle raises a xmlrpclib.ProtocolError when unavailable: <ProtocolError for api.opensubtitles.org/xml-rpc: 503 Backend fetch failed> Full traceback: Unexpected error in provider 'opensubtitles' Traceback (most recent call last): File "/home/osmc/Medusa/lib/subliminal/core.py", line 118, in list_subtitles_provider return self[provider].list_subtitles(video, provider_languages) File "/home/osmc/Medusa/lib/subliminal/core.py", line 68, in __getitem__ provider.initialize() File "/home/osmc/Medusa/lib/subliminal/providers/opensubtitles.py", line 142, in initialize 'subliminal v%s' % __short_version__)) File "/usr/lib/python2.7/xmlrpclib.py", line 1233, in __call__ return self.__send(self.__name, args) File "/usr/lib/python2.7/xmlrpclib.py", line 1591, in __request verbose=self.__verbose File "/usr/lib/python2.7/xmlrpclib.py", line 1273, in request return self.single_request(host, handler, request_body, verbose) File "/usr/lib/python2.7/xmlrpclib.py", line 1321, in single_request response.msg, ProtocolError: <ProtocolError for api.opensubtitles.org/xml-rpc: 503 Backend fetch failed>

Provider wrongful return a status of 304 Not Modified with an empty content raise_for_status won't raise exception for that status code

coveralls · 2017-08-02T14:36:45Z

Coverage decreased (-0.4%) to 91.187% when pulling 7682984 on fernandog:catch_unavailable into 2c34541 on Diaoul:develop.

coveralls · 2017-08-02T14:36:46Z

Coverage decreased (-0.4%) to 91.187% when pulling 7682984 on fernandog:catch_unavailable into 2c34541 on Diaoul:develop.

Diaoul

I don't like having the raise_for_status as a function as it only works for providers like requests and most importantly it makes adding new providers more difficult as you cannot just r.raise.. but have to use the utils.raise_for_status. I think the provider code should be as simple as possible, we can catch the requests Exception and check the status_code from within the exception handler instead. This would keep the initial intentions of this PR and reduce the footprint. What do you think?

fernandog · 2017-09-18T18:53:00Z

pymedusa/Medusa#3121

Implemented as you requested

fernandog force-pushed the catch_unavailable branch 3 times, most recently from 41d4a82 to 36ef828 Compare June 19, 2017 13:37

Diaoul reviewed Jun 20, 2017

View reviewed changes

fernandog commented Jun 20, 2017

View reviewed changes

fernandog force-pushed the catch_unavailable branch from 6df3f40 to 9b2d2c4 Compare July 6, 2017 12:00

fernandog force-pushed the catch_unavailable branch from 94802c2 to c483df4 Compare July 21, 2017 14:44

fernandog force-pushed the catch_unavailable branch 8 times, most recently from 9cd2368 to 69bcb87 Compare July 28, 2017 13:19

fernandog added 3 commits August 2, 2017 10:45

LegendasTV need a custom function as it returns 200 when unavailable

13cb24d

Proper handle empty content in Addic7ed

7682984

Provider wrongful return a status of 304 Not Modified with an empty content raise_for_status won't raise exception for that status code

fernandog force-pushed the catch_unavailable branch from 69bcb87 to 7682984 Compare August 2, 2017 13:45

Diaoul previously requested changes Aug 28, 2017

View reviewed changes

Review

1c1f2ce

Merge branch 'develop' into catch_unavailable

fce6f97

fernandog added the type/enhancement label Nov 18, 2017

fernandog merged commit 88ff505 into Diaoul:develop Nov 18, 2017

fernandog deleted the catch_unavailable branch November 18, 2017 12:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

When ServiceUnavailable, log error instead of exception (traceback) #764

When ServiceUnavailable, log error instead of exception (traceback) #764

fernandog commented Jun 19, 2017 •

edited

Diaoul Jun 20, 2017

fernandog Jun 20, 2017

Diaoul Jun 20, 2017 •

edited

fernandog Jun 20, 2017

Diaoul Jun 20, 2017

fernandog Jun 20, 2017

fernandog commented Jun 21, 2017 •

edited

pannal commented Jun 23, 2017 •

edited

fernandog commented Jun 23, 2017 •

edited

coveralls commented Jul 21, 2017

coveralls commented Jul 28, 2017 •

edited

coveralls commented Jul 28, 2017

coveralls commented Jul 28, 2017 •

edited

fernandog commented Jul 28, 2017

coveralls commented Aug 2, 2017 •

edited

coveralls commented Aug 2, 2017

Diaoul left a comment

fernandog commented Sep 18, 2017

When ServiceUnavailable, log error instead of exception (traceback) #764

When ServiceUnavailable, log error instead of exception (traceback) #764

Conversation

fernandog commented Jun 19, 2017 • edited

Diaoul Jun 20, 2017

Choose a reason for hiding this comment

fernandog Jun 20, 2017

Choose a reason for hiding this comment

Diaoul Jun 20, 2017 • edited

Choose a reason for hiding this comment

fernandog Jun 20, 2017

Choose a reason for hiding this comment

Diaoul Jun 20, 2017

Choose a reason for hiding this comment

fernandog Jun 20, 2017

Choose a reason for hiding this comment

fernandog commented Jun 21, 2017 • edited

pannal commented Jun 23, 2017 • edited

fernandog commented Jun 23, 2017 • edited

coveralls commented Jul 21, 2017

coveralls commented Jul 28, 2017 • edited

coveralls commented Jul 28, 2017

coveralls commented Jul 28, 2017 • edited

fernandog commented Jul 28, 2017

coveralls commented Aug 2, 2017 • edited

coveralls commented Aug 2, 2017

Diaoul left a comment

Choose a reason for hiding this comment

fernandog commented Sep 18, 2017

fernandog commented Jun 19, 2017 •

edited

Diaoul Jun 20, 2017 •

edited

fernandog commented Jun 21, 2017 •

edited

pannal commented Jun 23, 2017 •

edited

fernandog commented Jun 23, 2017 •

edited

coveralls commented Jul 28, 2017 •

edited

coveralls commented Jul 28, 2017 •

edited

coveralls commented Aug 2, 2017 •

edited