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

sitescripts/extensions/utils.py

Index: sitescripts/extensions/utils.py
===================================================================
--- a/sitescripts/extensions/utils.py
+++ b/sitescripts/extensions/utils.py
@@ -9,16 +9,17 @@
 # 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 contextlib
 import os
 import json
 import re
 import subprocess
 import traceback
 import time
 import urlparse
 import urllib
@@ -254,19 +255,18 @@
             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()
+        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

+            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