Noissue - use with and contextlib.closing for closing urlopen stream

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
@@ skipping 232 matching lines @@
 def _urlencode(value):
     return urllib.quote(value.encode('utf-8'), '')
 
 
 def _urlopen(url, attempts=3):
     """
     Tries to open a particular URL, retries on failure.
     """
     for i in range(attempts):
         try:
-            return urllib.urlopen(url)
+            return contextlib.closing(urllib.urlopen(url))
         except IOError as 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):
-        with contextlib.closing( _urlopen(url)) as page:

[Sebastian Noack, 2017/01/17 11:16:43]

Since we already have a wrapper for urlopen(), i.e. _urlopen(), perhaps we
should call contextlib.closing() there. So that if it is used more than once we
don't need to duplicate that call.

[Vasily Kuznetsov, 2017/01/17 11:39:13]

Yeah, that would be nice. However, the context manager returned by
`contextlib.closing` can't be used for reading directly, so then the only way to
use the result of `_urlopen()` will be with the `with` block and something like
`dom.parse(_urlopen(url))` will not be possible any more. At the moment it
doesn't matter since `_urlopen` is not used anywhere else but it seems we'd make
reuse more complicated. So I'd say it's more flexible the way it is and if we
end up using `_urlopen()` in other places, and they turn out to also be `with`
blocks, we will move `contextlib.closing` into it.

[Sebastian Noack, 2017/01/17 11:56:43]

I don't see any issues with that restriction. It should always be used with a
context manager anyway. Something like dom.parse(_urlopen(...)) is bad practice.
Instead, you should use a with-block and pass the response from there.

[Vasily Kuznetsov, 2017/01/17 13:13:51]

Fair enough, it is bad practice and in general I guess there's no harm if people
will be forced to use the result of `_urlopen` in a way that makes sure it's
closed at the end.

Done

+        with _urlopen(url) as page:
             content = page.read()
         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):
@@ skipping 120 matching lines @@
     if not extensions:
         return
 
     updates = {}
     for extension in extensions:
         updates[extension['basename']] = {
             'url': extension['updateURL'],
             'version': extension['version']
         }
     writeLibabpUpdateManifest(path, updates)