Merge lp:~jml/lazr.restfulclient/multiple-instance-safe into lp:lazr.restfulclient

[Jonathan Lange]

Creates AtomicFileCache as a parent class for MultipleRepresentationCache to handle concurrent use issues.

Hello,

I now work on a couple of projects that carry workarounds for bug 459418. It's not economical for my employer to be constantly maintaining these workarounds and to keep hitting this bug when writing new code that uses launchpadlib. As such, I offer this patch to make the file cache used by lazr.restfulclient safely shareable by multiple processes/threads.

It's an adaptation of the patch at http://code.google.com/p/httplib2/issues/detail?id=125.

It adds a new class, AtomicFileCache, and makes the MultipleRepresentationCache subclass that instead of the one that comes with httplib2.

Essentially, there are two components:

 1. Change look-before-you-leap to try/except. This prevents races where a thing exists (or doesn't exist) when we do the check, but then doesn't exist (or exists) when we do the operation.

 2. Do an atomic write in set, relying on the filesystem's atomic rename feature. This is not actually atomic on Windows, but will not be any buggier for them than the current code.

If you are interested in the subject of atomic writes, you might like to read <http://twistedmatrix.com/documents/current/api/twisted.python.filepath.FilePath.html#setContent>. The implementation of the method is much the same as set() here.

There are no tests. It's really hard to produce reliable tests for the race conditions that you find here without substantially changing the public API of the file cache. httplib2 doesn't have any tests for the interface of FileCache, otherwise I would have tried to re-use those. It wouldn't be too draconian to insist on adding some basic interface tests (e.g. does get() get what set() sets?) here, I think.

I look forward to hearing from you.

jml

[Richard Harding (rharding)]

Thanks for this.

review: Approve (code*)

[j.c.sackett (jcsackett)]

Jml:

This looks really great. I'm going to be that not-too-draconian reviewer and ask for the basic tests you've suggested.

I absolutely concur that creating a test for the race condition is out of consideration--the test case would like be sufficiently complicated that failures might be from a change to the test, not to the code being tested.

Next, I note several imports being whacked--I assume those are just not in use in the code anymore, correct?

Lastly, do you have landing rights for this, or will you need someone to push it along for you?

review: Needs Information

[Martin Pool (mbp)]

I'm very grateful for you addressing this bug.

This is very likely to fail on Windows due to the file being unable to be overwritten if it's in use. I know people have at least tried to use lazr.restfulclient and lplib on Windows. I don't know if it's actually supported or working. If it does work a bit, you might want to try not to regress it.

[Robert Collins (lifeless)]

+1 for the LoC increase per
https://dev.launchpad.net/PolicyAndProcess/MaintenanceCosts - this is
a significant source of friction in using lazr.restful.

-Rob

[lp:~jml/lazr.restfulclient/multiple-instance-safe]

131. By Jonathan Lange on 2012-03-23

Remove the destination file if the platform is windows (thanks mbp)

132. By Jonathan Lange on 2012-03-25

Initial tests for atomic file cache.

133. By Jonathan Lange on 2012-03-25

Interface tests for FileCache.

134. By Jonathan Lange on 2012-03-25

Extract out how we make the file cache.

135. By Jonathan Lange on 2012-03-25

Rename the test class.

136. By Jonathan Lange on 2012-03-25

Test things not being strings.

137. By Jonathan Lange on 2012-03-25

Tests for unicode behaviour.

138. By Jonathan Lange on 2012-03-25

Run the tests against httplib2.FileCache in order to prevent interface drift.

139. By Jonathan Lange on 2012-03-25

Copyright update.

140. By Jonathan Lange on 2012-03-25

Implementation-specific tests.

141. By Jonathan Lange on 2012-03-25

Docstrings.

[Jonathan Lange (jml)]

Hello all,

Thanks for the reviews, and for the free pass on increasing LoC.

I've added interface tests for FileCache and have set these up to run against both httplib2.FileCache and AtomicFileCache. The difference it makes to test run time is pretty small relative to current run time, and it will prevent interface drift.

Adding these tests revealed one difference between FileCache and AtomicFileCache: if FileCache fails to set(), say, because of a TypeError, then it will return '' on get() for that key; AtomicFileCache will return None, as if the key were never set. I think the difference in behaviour doesn't actually matter for our purposes, but I've left the tests in as documentation anyway. I also think that AtomicFileCache behaves correctly here and that FileCache itself is wrong.

I have also added docstrings, which I hope will help.

Per Martin's comment, I've updated set() to remove the file before renaming it, following the pattern laid out in twisted.python.filepath (which has had extensive usage and testing on Windows).

JC, yes, the removed imports are unused. If you don't believe me, well, I guess the automated test suite will catch me out ;)

I don't have sufficient permissions to land this myself, nor do I actually know how to land it. (Is it PQM-managed? Tarmac? etc.) I would be grateful if you would land it for me.

cheers,
jml

[j.c.sackett (jcsackett)]

> I've added interface tests for FileCache and have set these up to run against
> both httplib2.FileCache and AtomicFileCache. The difference it makes to test
> run time is pretty small relative to current run time, and it will prevent
> interface drift.
>
> Adding these tests revealed one difference between FileCache and
> AtomicFileCache: if FileCache fails to set(), say, because of a TypeError,
> then it will return '' on get() for that key; AtomicFileCache will return
> None, as if the key were never set. I think the difference in behaviour
> doesn't actually matter for our purposes, but I've left the tests in as
> documentation anyway. I also think that AtomicFileCache behaves correctly
> here and that FileCache itself is wrong.
>
> I have also added docstrings, which I hope will help.

Fantastic, thanks for those changes.

> JC, yes, the removed imports are unused. If you don't believe me, well, I
> guess the automated test suite will catch me out ;)

I believe you; I couldn't get at the code myself on Thursday for reasons best attributed to massive ISP fail, so fell back on the old method of asking. :-p

> I don't have sufficient permissions to land this myself, nor do I actually
> know how to land it. (Is it PQM-managed? Tarmac? etc.) I would be grateful if
> you would land it for me.

I will see what I can do; I think this is something we certainly want landed swiftly.
> cheers,
> jml

review: Approve

[Jonathan Lange (jml)]

On 26 March 2012 15:16, j.c.sackett <email address hidden> wrote:
...
>> I don't have sufficient permissions to land this myself, nor do I actually
>> know how to land it. (Is it PQM-managed? Tarmac? etc.)  I would be grateful if
>> you would land it for me.
>
> I will see what I can do; I think this is something we certainly want landed swiftly.

Thanks! A release would help a lot too.

jml

[Jonathan Lange (jml)]

How's it going getting this landed? (It's filling up a WIP slot on our kanban).

[Martin Pool (mbp)]

I don't want to stall this, but I do think that

    os.unlink(cache_full_path)

may still fail on Windows if the existing file is in use by some other client. Solutions could include:

 - just log it (perhaps silent by default) and continue
 - warn
 - retry for a bit (not great because it slows down the writer)
 - change to using win32 apis that don't lock against deletion

I don't know how well this works on Windows at the moment, but if people are using it this might be a regression.

[Robert Collins (lifeless)]

On Mon, Apr 2, 2012 at 5:24 PM, Martin Pool <email address hidden> wrote:
> I don't want to stall this, but I do think that
>
>    os.unlink(cache_full_path)
>
> may still fail on Windows if the existing file is in use by some other client.  Solutions could include:
>
>  - just log it (perhaps silent by default) and continue
>  - warn
>  - retry for a bit (not great because it slows down the writer)
>  - change to using win32 apis that don't lock against deletion
>
> I don't know how well this works on Windows at the moment, but if people are using it this might be a regression.

It will die horribly at the moment, this isn't going to be a
regression: we don't currently support concurrent use. This patch will
give us concurrent use on Linux. A future iteration can do concurrent
use on win32.

-Rob

=== modified file 'buildout.cfg'
--- buildout.cfg	2011-11-18 20:23:48 +0000
+++ buildout.cfg	2012-03-25 14:24:17 +0000
@@ -9,6 +9,7 @@
 unzip = true
 develop = .
 include-site-packages = false
+download-cache = download-cache
 
 [test]
 recipe = zc.recipe.testrunner

=== modified file 'src/lazr/restfulclient/_browser.py'
--- src/lazr/restfulclient/_browser.py	2011-06-28 05:15:02 +0000
+++ src/lazr/restfulclient/_browser.py	2012-03-25 14:24:17 +0000
@@ -1,4 +1,4 @@
-# Copyright 2008 Canonical Ltd.
+# Copyright 2008,2012 Canonical Ltd.
 
 # This file is part of lazr.restfulclient.
 #
@@ -29,19 +29,21 @@
     'RestfulHttp',
     ]
 
-
 import atexit
-import gzip
+import errno
+import os
 import shutil
+import sys
 import tempfile
 # Import sleep directly into the module so we can monkey-patch it
 # during a test.
 from time import sleep
 from httplib2 import (
-    FailedToDecompressContent, FileCache, Http, urlnorm)
+    Http,
+    urlnorm,
+    )
 import simplejson
 from cStringIO import StringIO
-import zlib
 
 from urllib import urlencode
 from wadllib.application import Application
@@ -49,6 +51,7 @@
 from errors import error_for, HTTPError
 from _json import DatetimeJSONEncoder
 
+
 # A drop-in replacement for httplib2's safename.
 from httplib2 import _md5, re_url_scheme, re_slash
 def safename(filename):
@@ -136,7 +139,99 @@
         return None
 
 
-class MultipleRepresentationCache(FileCache):
+class AtomicFileCache(object):
+    """A FileCache that can be shared by multiple processes.
+
+    Based on a patch found at
+    <http://code.google.com/p/httplib2/issues/detail?id=125>.
+    """
+
+    TEMPFILE_PREFIX = ".temp"
+
+    def __init__(self, cache, safe=safename):
+        """Construct an ``AtomicFileCache``.
+
+        :param cache: The directory to use as a cache.
+        :param safe: A function that takes a key and returns a name that's
+            safe to use as a filename.  The key must never return a string
+            that begins with ``TEMPFILE_PREFIX``.  By default uses
+            ``safename``.
+        """
+        self._cache_dir = os.path.normpath(cache)
+        self._get_safe_name = safe
+        try:
+             os.makedirs(self._cache_dir)
+        except OSError, e:
+            if e.errno != errno.EEXIST:
+                raise
+
+    def _get_key_path(self, key):
+        """Return the path on disk where ``key`` is stored."""
+        safe_key = self._get_safe_name(key)
+        if safe_key.startswith(self.TEMPFILE_PREFIX):
+            # If the cache key starts with the tempfile prefix, then it's
+            # possible that it will clash with a temporary file that we
+            # create.
+            raise ValueError(
+                "Cache key cannot start with '%s'" % self.TEMPFILE_PREFIX)
+        return os.path.join(self._cache_dir, safe_key)
+
+    def get(self, key):
+        """Get the value of ``key`` if set.
+
+        This behaves slightly differently to ``FileCache`` in that if
+        ``set()`` fails to store a key, this ``get()`` will behave as if that
+        key were never set whereas ``FileCache`` returns the empty string.
+
+        :param key: The key to retrieve.  Must be either bytes or unicode
+            text.
+        :return: The value of ``key`` if set, None otherwise.
+        """
+        cache_full_path = self._get_key_path(key)
+        try:
+            f = open(cache_full_path, 'rb')
+            try:
+                return f.read()
+            finally:
+                f.close()
+        except (IOError, OSError), e:
+            if e.errno != errno.ENOENT:
+                raise
+
+    def set(self, key, value):
+        """Set ``key`` to ``value``.
+
+        :param key: The key to set.  Must be either bytes or unicode text.
+        :param value: The value to set ``key`` to.  Must be bytes.
+        """
+        # Open a temporary file
+        handle, path_name = tempfile.mkstemp(
+            prefix=self.TEMPFILE_PREFIX, dir=self._cache_dir)
+        f = os.fdopen(handle, 'wb')
+        f.write(value)
+        f.close()
+        cache_full_path = self._get_key_path(key)
+        # And rename atomically (on POSIX at least)
+        if sys.platform == 'win32' and os.path.exists(cache_full_path):
+            os.unlink(cache_full_path)
+        os.rename(path_name, cache_full_path)
+
+    def delete(self, key):
+        """Delete ``key`` from the cache.
+
+        If ``key`` has not already been set then has no effect.
+
+        :param key: The key to delete.  Must be either bytes or unicode text.
+        """
+        cache_full_path = self._get_key_path(key)
+        try:
+            os.remove(cache_full_path)
+        except OSError, e:
+            if e.errno != errno.ENOENT:
+                raise
+
+
+class MultipleRepresentationCache(AtomicFileCache):
     """A cache that can hold different representations of the same resource.
 
     If a resource has two representations with two media types,

=== added file 'src/lazr/restfulclient/tests/test_atomicfilecache.py'
--- src/lazr/restfulclient/tests/test_atomicfilecache.py	1970-01-01 00:00:00 +0000
+++ src/lazr/restfulclient/tests/test_atomicfilecache.py	2012-03-25 14:24:17 +0000
@@ -0,0 +1,158 @@
+# Copyright 2012 Canonical Ltd.
+
+# This file is part of lazr.restfulclient.
+#
+# lazr.restfulclient is free software: you can redistribute it and/or
+# modify it under the terms of the GNU Lesser General Public License
+# as published by the Free Software Foundation, version 3 of the
+# License.
+#
+# lazr.restfulclient 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
+# Lesser General Public License for more details.
+#
+# You should have received a copy of the GNU Lesser General Public License
+# along with lazr.restfulclient. If not, see <http://www.gnu.org/licenses/>.
+
+"""Tests for the atomic file cache."""
+
+__metaclass__ = type
+
+import shutil
+import tempfile
+import unittest
+
+import httplib2
+
+from lazr.restfulclient._browser import AtomicFileCache
+
+
+class TestFileCacheInterface(unittest.TestCase):
+    """Tests for ``AtomicFileCache``."""
+
+    file_cache_factory = httplib2.FileCache
+
+    unicode_text = u'pa\u026a\u03b8\u0259n'
+
+    def setUp(self):
+        super(TestFileCacheInterface, self).setUp()
+        self.cache_dir = tempfile.mkdtemp()
+
+    def tearDown(self):
+        shutil.rmtree(self.cache_dir)
+        super(TestFileCacheInterface, self).tearDown()
+
+    def make_file_cache(self):
+        """Make a FileCache-like object to be tested."""
+        return self.file_cache_factory(self.cache_dir)
+
+    def test_get_non_existent_key(self):
+        # get() returns None if the key does not exist.
+        cache = self.make_file_cache()
+        self.assertIs(None, cache.get('nonexistent'))
+
+    def test_set_key(self):
+        # A key set with set() can be got by get().
+        cache = self.make_file_cache()
+        cache.set('key', 'value')
+        self.assertEqual('value', cache.get('key'))
+
+    def test_set_twice_overrides(self):
+        # Setting a key again overrides the value.
+        cache = self.make_file_cache()
+        cache.set('key', 'value')
+        cache.set('key', 'new-value')
+        self.assertEqual('new-value', cache.get('key'))
+
+    def test_delete_absent_key(self):
+        # Deleting a key that's not there does nothing.
+        cache = self.make_file_cache()
+        cache.delete('nonexistent')
+        self.assertIs(None, cache.get('nonexistent'))
+
+    def test_delete_key(self):
+        # A key once set can be deleted.  Further attempts to get that key
+        # return None.
+        cache = self.make_file_cache()
+        cache.set('key', 'value')
+        cache.delete('key')
+        self.assertIs(None, cache.get('key'))
+
+    def test_get_non_string_key(self):
+        # get() raises TypeError if asked to get a non-string key.
+        cache = self.make_file_cache()
+        self.assertRaises(TypeError, cache.get, 42)
+
+    def test_delete_non_string_key(self):
+        # delete() raises TypeError if asked to delete a non-string key.
+        cache = self.make_file_cache()
+        self.assertRaises(TypeError, cache.delete, 42)
+
+    def test_set_non_string_key(self):
+        # set() raises TypeError if asked to set a non-string key.
+        cache = self.make_file_cache()
+        self.assertRaises(TypeError, cache.set, 42, 'the answer')
+
+    def test_set_non_string_value(self):
+        # set() raises TypeError if asked to set a key to a non-string value.
+        # Attempts to retrieve that value return the empty string.  This is
+        # probably a bug in httplib2.FileCache.
+        cache = self.make_file_cache()
+        self.assertRaises(TypeError, cache.set, 'answer', 42)
+        self.assertEqual('', cache.get('answer'))
+
+    def test_get_unicode(self):
+        # get() can retrieve unicode keys.
+        cache = self.make_file_cache()
+        self.assertIs(None, cache.get(self.unicode_text))
+
+    def test_set_unicode_keys(self):
+        cache = self.make_file_cache()
+        cache.set(self.unicode_text, 'value')
+        self.assertEqual('value', cache.get(self.unicode_text))
+
+    def test_set_unicode_value(self):
+        # set() cannot store unicode values.  Values must be bytes.
+        cache = self.make_file_cache()
+        self.assertRaises(
+            UnicodeEncodeError, cache.set, 'key', self.unicode_text)
+
+    def test_delete_unicode(self):
+        # delete() can remove unicode keys.
+        cache = self.make_file_cache()
+        cache.set(self.unicode_text, 'value')
+        cache.delete(self.unicode_text)
+        self.assertIs(None, cache.get(self.unicode_text))
+
+
+class TestAtomicFileCache(TestFileCacheInterface):
+    """Tests for ``AtomicFileCache``."""
+
+    file_cache_factory = AtomicFileCache
+
+    def test_set_non_string_value(self):
+        # set() raises TypeError if asked to set a key to a non-string value.
+        # Attempts to retrieve that value act is if it were never set.
+        #
+        # Note: This behaviour differs from httplib2.FileCache.
+        cache = self.make_file_cache()
+        self.assertRaises(TypeError, cache.set, 'answer', 42)
+        self.assertIs(None, cache.get('answer'))
+
+    # Implementation-specific tests follow.
+
+    def test_bad_safename_get(self):
+        safename = lambda x: AtomicFileCache.TEMPFILE_PREFIX + x
+        cache = AtomicFileCache(self.cache_dir, safename)
+        self.assertRaises(ValueError, cache.get, 'key')
+
+    def test_bad_safename_set(self):
+        safename = lambda x: AtomicFileCache.TEMPFILE_PREFIX + x
+        cache = AtomicFileCache(self.cache_dir, safename)
+        self.assertRaises(ValueError, cache.set, 'key', 'value')
+
+    def test_bad_safename_delete(self):
+        safename = lambda x: AtomicFileCache.TEMPFILE_PREFIX + x
+        cache = AtomicFileCache(self.cache_dir, safename)
+        self.assertRaises(ValueError, cache.delete, 'key')