Issue 4767 - Improve error reporting in update_update_manifests

sitescripts/extensions/utils.py

 # This file is part of the Adblock Plus web scripts,
 # Copyright (C) 2006-2016 Eyeo GmbH
 #
 # Adblock Plus is free software: you can redistribute it and/or modify
 # it under the terms of the GNU General Public License version 3 as
 # published by the Free Software Foundation.
 #
 # Adblock Plus is distributed in the hope that it will be useful,
 # but WITHOUT ANY WARRANTY; without even the implied warranty of
 # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 # GNU General Public License for more details.
 #
 # You should have received a copy of the GNU General Public License
 # along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
 
 import codecs
 import os
 import json
 import re
 import subprocess
 import traceback
 import time
 import urlparse
 import urllib
 import xml.dom.minidom as dom
-import sys
 from ConfigParser import SafeConfigParser, NoOptionError
 from StringIO import StringIO
 from sitescripts.utils import get_config
 from xml.parsers.expat import ExpatError
 
 PACKAGE_SUFFIXES = {
     'gecko': '.xpi',
     'gecko-webext': '.xpi',
     'chrome': '.crx',
     'safari': '.safariextz',
@@ skipping 208 matching lines @@
 
 
 def _urlopen(url, attempts=3):
     """
     Tries to open a particular URL, retries on failure.
     """
     for i in range(attempts):
         try:
             return urllib.urlopen(url)
         except IOError as e:
-            error = e
+            error = Exception('Error {0} while opening {1} url'
+                              .format(e, url))
             time.sleep(5)
     raise error
 
 
+def _parseXMLDocument(url, attempts=2):
+    for i in range(attempts):
+        page = _urlopen(url)
+        content = page.read()
+        page.close()

[Sebastian Noack, 2017/01/17 09:33:32]

The close() should have gone into a finally block, or even better using the with
statement:

  with contextlib.closing(_urlopen(url)) as page:
      content = page.read()

[Sebastian Noack, 2017/01/17 10:16:56]

Yes, in Python 2, the repsonse object returned by urlopen() doesn't implement
__enter__/__exit__. So you either have to use the contextlib.closing() decorator
before passing it to the with statement or use a traditionally try-finally
block. I suggest doing the former, and removing the decorator once we migrate to
Python 3.

Also, please reply inline to inline comment.

[Sebastian Noack, 2017/01/17 10:20:15]

Well, alternatively you could use try-finally. But IMO, the with-statement is
nicer. For reference, here is how we did it in jsHydra where we support both
Python 2+3: https://hg.adblockplus.org/jshydra/file/tip/abp_rewrite.py#l11

[Vasily Kuznetsov, 2017/01/17 10:30:34]

Yeah, with is nicer, especially having Python 3 in mind.

+        try:
+            return dom.parseString(content)
+        except ExpatError as err:
+            exception = Exception('Error {0} while parsing xml:\n{1}\nfrom {2}'
+                                  .format(err, content, url))
+    raise exception
+
+
 def _getMozillaDownloadLink(galleryID):
     """
     gets download link for a Gecko add-on from the Mozilla Addons site
     """
-    try:
-        url = 'https://services.addons.mozilla.org/en-US/firefox/api/1/addon/%s' % _urlencode(galleryID)
-        document = dom.parse(_urlopen(url))

[Vasily Kuznetsov, 2017/01/12 11:12:32]

Actually we don't need to put the whole block into try/except to catch
ExpatError. Expat errors are only created by `dom.parse`. So you can put only
this one line into try/except.

If you want to re-download and re-parse once (as we dicussed over IRC), it would
probably make more sense to move this line into a separate function similar to
`_urlopen`.

-        linkTags = document.getElementsByTagName('install')
-        linkTag = linkTags[0] if len(linkTags) > 0 else None
-        versionTags = document.getElementsByTagName('version')
-        versionTag = versionTags[0] if len(versionTags) > 0 else None
-        if linkTag and versionTag and linkTag.firstChild and versionTag.firstChild:
-            return (linkTag.firstChild.data, versionTag.firstChild.data)
-        else:
-            return (None, None)
-    except ExpatError:
-        page = _urlopen(url)

[Vasily Kuznetsov, 2017/01/12 11:12:32]

This is not a good idea. You're downloading the page again just to put it into
the exception text. First of all, it's wasteful, we've already downloaded it on
line 266, no need to do it again. Second, it can lead to very misleading error
messages if what you download on the second try turns out to be good XML.

So as per comment in line 266, it would be better to move parsing and its error
reporting into a separate function and there do something like:

for i in range(attempts):
    with _urlopen(url) as page:
        content = page.read()
    try:
        return dom.parseString(content)
    except err:
        # We shouldn't raise this immediately since we'll be retrying.
        exception = Exception('error {} in parsing xml:\n{}\nfrom
{}'.format(err, content, url))
raise exception

-        content = document.open()
-        page.close()
-        raise Exception('Error found while parsing xml:\n{0}\nfrom {1} link'
-                        .format(content, galleryID))
-
+    url = 'https://services.addons.mozilla.org/en-US/firefox/api/1/addon/%s' % _urlencode(galleryID)
+    document = _parseXMLDocument(url)
+    linkTags = document.getElementsByTagName('install')
+    linkTag = linkTags[0] if len(linkTags) > 0 else None
+    versionTags = document.getElementsByTagName('version')
+    versionTag = versionTags[0] if len(versionTags) > 0 else None
+    if linkTag and versionTag and linkTag.firstChild and versionTag.firstChild:
+        return (linkTag.firstChild.data, versionTag.firstChild.data)
+    else:
+        return (None, None)
 
 
 def _getLocalLink(repo):
     """
     gets the link for the newest download of an add-on in the local downloads
     repository
     """
     highestURL = None
     highestVersion = None
 
@@ skipping 97 matching lines @@
     if not extensions:
         return
 
     updates = {}
     for extension in extensions:
         updates[extension['basename']] = {
             'url': extension['updateURL'],
             'version': extension['version']
         }
     writeLibabpUpdateManifest(path, updates)