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
 # 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 os
 import subprocess
 import codecs
+import contextlib
 import urllib2
 import zipfile
 import tempfile
 import shutil
 import sys
 from StringIO import StringIO
 from sitescripts.utils import get_config
 
 
 FILTERLIST_HEADER = '''[Adblock Plus 1.1]
 ! This is a list of malware domains generated from malwaredomains.com data.
 ! Homepage: http://malwaredomains.com/?page_id=2
 ! Last modified: %timestamp%
 ! Expires: 1d
 !'''
 
 MALWAREDOMAINS_PATH = '/files/justdomains.zip'
 
 
 def try_mirror(mirror):
+    url = mirror + MALWAREDOMAINS_PATH
     try:
-        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()
+        with contextlib.closing(urllib2.urlopen(url)) as response:
+            return None, response.read()
+    except urllib2.HTTPError as exc:
+        exc.close()
     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
+        pass
+    return 'Failed to fetch {}: {}'.format(url, exc), None
 
 
 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:
@@ skipping 26 matching lines @@
 
         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()