Fixes 4784 - Improve error reporting in updateMalwareDomainsList and add tests

sitescripts/subscriptions/bin/updateMalwareDomainsList.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
@@ skipping 19 matching lines @@
 ! Homepage: http://malwaredomains.com/?page_id=2
 ! Last modified: %timestamp%
 ! Expires: 1d
 !'''
 
 MALWAREDOMAINS_PATH = '/files/justdomains.zip'
 
 
 def try_mirror(mirror):
     try:
-        response = urllib2.urlopen(mirror + MALWAREDOMAINS_PATH)
-        return response.read()
-    except urllib2.HTTPError:
-        return None
+        url = mirror + MALWAREDOMAINS_PATH

[Sebastian Noack, 2017/01/19 09:01:29]

This line doesn't seem to belong in to the try-block (there is no way that it
can potentially throw a URLError.

[Vasily Kuznetsov, 2017/01/19 16:34:08]

Done.

+        response = urllib2.urlopen(url)

[Sebastian Noack, 2017/01/19 09:01:29]

I think we should explicitly close the response.

[Jon Sonesen, 2017/01/19 09:31:24]

Could this be resolved by using a 'with' statement to open it. This way it
closes even in the event of an exception?

[Sebastian Noack, 2017/01/19 09:48:18]

Yes, this should be implemented either using try-finally or using a
with-statement, so that it gets closed no matter what. However, in Python 2, the
response returned by urlopen() doesn't implement __enter__/__exit__. So in order
to use the with-statement here, the response would have to be wrapped with
contextlib.closing(). Moreover, if an HTTPError is raised (see below), that
exception is a file-like object itself, which needs to be closed as well.

On 2017/01/19 09:33:53, Jon Sonesen wrote:
Relying on that behavior is bad practice. First of all this is an implementation
detail of CPython, and as such it should not be relied on. If possible we should
target the Python language, not a specific implementation. More importantly,
__del__ methods are resolved by the garbage collector in CPython, and therefore
closing the response would be indefinitely delayed, so that most likely the
response will still be open while we already send subsequent requests.

[Jon Sonesen, 2017/01/19 09:55:01]

Thanks for elaborating on those points :) 

[Vasily Kuznetsov, 2017/01/19 16:34:08]

Done.

+        return None, response.read()
+    except urllib2.URLError as exc:

[Sebastian Noack, 2017/01/19 09:01:29]

Note that HTTPError (a subclass of URLError) is also a file-like object that
should be closed.

[Vasily Kuznetsov, 2017/01/19 16:34:08]

Done.

+        error_message = 'Failed to fetch {}: {}'.format(url, exc)

[Jon Sonesen, 2017/01/18 17:04:48]

Perhaps it is more useful to you the reason attribute here rather than the
string representation of the exception as per the urllib2 docs for URLErrors

[Vasily Kuznetsov, 2017/01/18 18:46:35]

We might also catch subclasses of URLError here. Basically it means HTTPError,
which also has error code in .code attribute. I would like to include the error
code into error_message and just including exc.reason would not do it. The
__str__ methods of URLError and HTTPError do include all relevant information,
so with my approach I don't need to worry about what is the exact exception
class. The disadvantage here is that those two __str__ methods are not
consistent in terms of how the output is formatted, but I think we can live with
it. So all in all I'd prefer to keep this line the way it is if that's ok.

+        return error_message, None
 
 
-if __name__ == '__main__':
+def main():
     config = get_config()
     section = 'subscriptionDownloads'
     repository = config.get(section, 'malwaredomains_repository')
     mirrors = config.get(section, 'malwaredomains_mirrors').split()
 
     tempdir = tempfile.mkdtemp(prefix='malwaredomains')
     try:
         subprocess.check_call(['hg', '-q', 'clone', '-U', repository, tempdir])
         subprocess.check_call(['hg', '-q', 'up', '-R', tempdir, '-r', 'default'])
 
         path = os.path.join(tempdir, 'malwaredomains_full.txt')
         file = codecs.open(path, 'wb', encoding='utf-8')
 
         print >>file, FILTERLIST_HEADER
 
+        error_report = ['Unable to fetch malware domains list', 'Errors:']
         for mirror in mirrors:
-            data = try_mirror(mirror)
+            error_message, data = try_mirror(mirror)
             if data is not None:
                 break
+            error_report.append(error_message)
         else:
-            sys.exit('Unable to fetch malware domains list.')
+            sys.exit('\n'.join(error_report))
 
-        zip = zipfile.ZipFile(StringIO(data), 'r')

[Vasily Kuznetsov, 2017/01/10 18:18:29]

These three lines are an unrelated change. Flake8 started complaining about A302
here (not sure how it passed before) so I fixed it.

-        info = zip.infolist()[0]
-        for line in str(zip.read(info.filename)).splitlines():
+        zf = zipfile.ZipFile(StringIO(data), 'r')
+        info = zf.infolist()[0]
+        for line in str(zf.read(info.filename)).splitlines():
             domain = line.strip()
             if not domain:
                 continue
 
             print >>file, '||%s^' % domain.decode('idna')
         file.close()
 
         if subprocess.check_output(['hg', 'stat', '-R', tempdir]) != '':
             subprocess.check_call(['hg', '-q', 'commit', '-R', tempdir, '-A', '-u', 'hgbot', '-m', 'Updated malwaredomains.com data'])
             subprocess.check_call(['hg', '-q', 'push', '-R', tempdir])
     finally:
         shutil.rmtree(tempdir, ignore_errors=True)
+
+
+if __name__ == '__main__':
+    main()