Merge lp:~verterok/ubuntuone-client/fix-987376 into lp:ubuntuone-client

[Guillermo Gonzalez]

Only use mmap in files of size < 2**31, in order to avoid hitting address space limits, and fallback to standard IO.

Only use mmap in files of size < 2**31, in order to avoid hitting address space limits, and fallback to standard IO.
I'll be working on limiting the file growth in a following branch, to fix Bug #987382.

[Facundo Batista (facundo)]

Thanks!

review: Approve

[Manuel de la Peña (mandel)]

The following happens on widows:

[ERROR]
Traceback (most recent call last):
  File "C:\Users\Mandel\Projects\Canonical\ubuntuone-client\fix-987376\tests\syn
cdaemon\test_tritcask.py", line 214, in test_iter_entries_bad_crc
    for i, entry in enumerate(db.live_file.iter_entries()):
  File "C:\Users\Mandel\Projects\Canonical\ubuntuone-client\fix-987376\ubuntuone
\syncdaemon\tritcask.py", line 257, in iter_entries
    for entry in self._iter_entries(fmmap):
  File "C:\Users\Mandel\Projects\Canonical\ubuntuone-client\fix-987376\ubuntuone
\syncdaemon\tritcask.py", line 266, in _iter_entries
    entry, new_pos = self.read(fd)
  File "C:\Users\Mandel\Projects\Canonical\ubuntuone-client\fix-987376\ubuntuone
\syncdaemon\tritcask.py", line 323, in read
    data = fd.read(key_sz+value_sz)
exceptions.OverflowError: Python int too large to convert to C long

tests.syncdaemon.test_tritcask.DataFileTest.test_iter_entries_bad_crc
===============================================================================
[ERROR]
Traceback (most recent call last):
  File "C:\Users\Mandel\Projects\Canonical\ubuntuone-client\fix-987376\tests\syn
cdaemon\test_tritcask.py", line 214, in test_iter_entries_bad_crc
    for i, entry in enumerate(db.live_file.iter_entries()):
  File "C:\Users\Mandel\Projects\Canonical\ubuntuone-client\fix-987376\ubuntuone
\syncdaemon\tritcask.py", line 252, in iter_entries
    for entry in self._iter_entries(self.fd):
  File "C:\Users\Mandel\Projects\Canonical\ubuntuone-client\fix-987376\ubuntuone
\syncdaemon\tritcask.py", line 266, in _iter_entries
    entry, new_pos = self.read(fd)
  File "C:\Users\Mandel\Projects\Canonical\ubuntuone-client\fix-987376\ubuntuone
\syncdaemon\tritcask.py", line 323, in read
    data = fd.read(key_sz+value_sz)
exceptions.MemoryError:

tests.syncdaemon.test_tritcask.NoMmapDataFileTest.test_iter_entries_bad_crc

review: Needs Fixing

[Guillermo Gonzalez (verterok)]

Thanks for spotting that one.
It's fixed and pushed.

[Manuel de la Peña (mandel)]

Everything works in all currently supported platforms!

review: Approve

[Ubuntu One Auto Pilot (otto-pilot)]

The attempt to merge lp:~verterok/ubuntuone-client/fix-987376 into lp:ubuntuone-client failed. Below is the output from the failed tests.

/usr/bin/gnome-autogen.sh
checking for autoconf >= 2.53...
  testing autoconf2.50... not found.
  testing autoconf... found 2.68
checking for automake >= 1.10...
  testing automake-1.11... found 1.11.3
checking for libtool >= 1.5...
  testing libtoolize... found 2.4.2
checking for intltool >= 0.30...
  testing intltoolize... found 0.50.2
checking for pkg-config >= 0.14.0...
  testing pkg-config... found 0.26
checking for gtk-doc >= 1.0...
  testing gtkdocize... found 1.18
Checking for required M4 macros...
Checking for forbidden M4 macros...
Processing ./configure.ac
Running libtoolize...
libtoolize: putting auxiliary files in `.'.
libtoolize: copying file `./ltmain.sh'
libtoolize: putting macros in AC_CONFIG_MACRO_DIR, `m4'.
libtoolize: copying file `m4/libtool.m4'
libtoolize: copying file `m4/ltoptions.m4'
libtoolize: copying file `m4/ltsugar.m4'
libtoolize: copying file `m4/ltversion.m4'
libtoolize: copying file `m4/lt~obsolete.m4'
Running intltoolize...
Running gtkdocize...
Running aclocal-1.11...
Running autoconf...
Running autoheader...
Running automake-1.11...
Running ./configure --enable-gtk-doc --enable-debug ...
checking for a BSD-compatible install... /usr/bin/install -c
checking whether build environment is sane... yes
checking for a thread-safe mkdir -p... /bin/mkdir -p
checking for gawk... no
checking for mawk... mawk
checking whether make sets $(MAKE)... yes
checking whether make supports nested variables... yes
checking for style of include used by make... GNU
checking for gcc... gcc
checking whether the C compiler works... yes
checking for C compiler default output file name... a.out
checking for suffix of executables...
checking whether we are cross compiling... no
checking for suffix of object files... o
checking whether we are using the GNU C compiler... yes
checking whether gcc accepts -g... yes
checking for gcc option to accept ISO C89... none needed
checking dependency style of gcc... gcc3
checking for library containing strerror... none required
checking for gcc... (cached) gcc
checking whether we are using the GNU C compiler... (cached) yes
checking whether gcc accepts -g... (cached) yes
checking for gcc option to accept ISO C89... (cached) none needed
checking dependency style of gcc... (cached) gcc3
checking build system type... i686-pc-linux-gnu
checking host system type... i686-pc-linux-gnu
checking how to print strings... printf
checking for a sed that does not truncate output... /bin/sed
checking for grep that handles long lines and -e... /bin/grep
checking for egrep... /bin/grep -E
checking for fgrep... /bin/grep -F
checking for ld used by gcc... /usr/bin/ld
checking if the linker (/usr/bin/ld) is GNU ld... yes
checking for BSD- or MS-compatible name lister (nm)... /usr/bin/nm -B
checking the name lister (/usr/bin/nm -B) interface... BSD nm
checking whether ln -s works... yes
checking the maximum length of command line arguments... 1572864
checking whether the shell understands some XSI constructs... yes
checking whether the shell understands "+="... yes
checking how to con...

[Facundo Batista (facundo)]

Tested again latest changes.

review: Approve

[Ubuntu One Auto Pilot (otto-pilot)]

The attempt to merge lp:~verterok/ubuntuone-client/fix-987376 into lp:ubuntuone-client failed. Below is the output from the failed tests.

/usr/bin/gnome-autogen.sh
checking for autoconf >= 2.53...
  testing autoconf2.50... not found.
  testing autoconf... found 2.68
checking for automake >= 1.10...
  testing automake-1.11... found 1.11.3
checking for libtool >= 1.5...
  testing libtoolize... found 2.4.2
checking for intltool >= 0.30...
  testing intltoolize... found 0.50.2
checking for pkg-config >= 0.14.0...
  testing pkg-config... found 0.26
checking for gtk-doc >= 1.0...
  testing gtkdocize... found 1.18
Checking for required M4 macros...
Checking for forbidden M4 macros...
Processing ./configure.ac
Running libtoolize...
libtoolize: putting auxiliary files in `.'.
libtoolize: copying file `./ltmain.sh'
libtoolize: putting macros in AC_CONFIG_MACRO_DIR, `m4'.
libtoolize: copying file `m4/libtool.m4'
libtoolize: copying file `m4/ltoptions.m4'
libtoolize: copying file `m4/ltsugar.m4'
libtoolize: copying file `m4/ltversion.m4'
libtoolize: copying file `m4/lt~obsolete.m4'
Running intltoolize...
Running gtkdocize...
Running aclocal-1.11...
Running autoconf...
Running autoheader...
Running automake-1.11...
Running ./configure --enable-gtk-doc --enable-debug ...
checking for a BSD-compatible install... /usr/bin/install -c
checking whether build environment is sane... yes
checking for a thread-safe mkdir -p... /bin/mkdir -p
checking for gawk... no
checking for mawk... mawk
checking whether make sets $(MAKE)... yes
checking whether make supports nested variables... yes
checking for style of include used by make... GNU
checking for gcc... gcc
checking whether the C compiler works... yes
checking for C compiler default output file name... a.out
checking for suffix of executables...
checking whether we are cross compiling... no
checking for suffix of object files... o
checking whether we are using the GNU C compiler... yes
checking whether gcc accepts -g... yes
checking for gcc option to accept ISO C89... none needed
checking dependency style of gcc... gcc3
checking for library containing strerror... none required
checking for gcc... (cached) gcc
checking whether we are using the GNU C compiler... (cached) yes
checking whether gcc accepts -g... (cached) yes
checking for gcc option to accept ISO C89... (cached) none needed
checking dependency style of gcc... (cached) gcc3
checking build system type... i686-pc-linux-gnu
checking host system type... i686-pc-linux-gnu
checking how to print strings... printf
checking for a sed that does not truncate output... /bin/sed
checking for grep that handles long lines and -e... /bin/grep
checking for egrep... /bin/grep -E
checking for fgrep... /bin/grep -F
checking for ld used by gcc... /usr/bin/ld
checking if the linker (/usr/bin/ld) is GNU ld... yes
checking for BSD- or MS-compatible name lister (nm)... /usr/bin/nm -B
checking the name lister (/usr/bin/nm -B) interface... BSD nm
checking whether ln -s works... yes
checking the maximum length of command line arguments... 1572864
checking whether the shell understands some XSI constructs... yes
checking whether the shell understands "+="... yes
checking how to con...

Unmerged revisions

1234. By Guillermo Gonzalez on 2012-05-08

fix tests

1233. By Guillermo Gonzalez on 2012-05-04

fix bad_crc tests to run on windows.

1232. By Guillermo Gonzalez on 2012-04-25

don't iterate the generator in DataFile.iter_entries, just return it.

1231. By Guillermo Gonzalez on 2012-04-24

fix max mmaped file limit

1230. By Guillermo Gonzalez on 2012-04-24

only use mmap in data files <= 2**32, to avoid hitting address space limits, and fallback to standard IO.

=== modified file 'tests/syncdaemon/test_tritcask.py'
--- tests/syncdaemon/test_tritcask.py	2012-04-09 20:07:05 +0000
+++ tests/syncdaemon/test_tritcask.py	2012-05-08 14:03:17 +0000
@@ -205,8 +205,8 @@
             db.put(i, 'foo%d' % (i,), 'bar%s' % (i,))
         # write a different value -> random bytes
         # now write some garbage to the end of file
-        db.live_file.fd.write(os.urandom(100))
-        db.live_file.fd.flush()
+        db.live_file.fd.seek(db.live_file.fd.tell()-3)
+        db.live_file.fd.write(os.urandom(3))
         # and add 10 new entries
         for i in range(10, 20):
             db.put(i, 'foo%d' % (i,), 'bar%s' % (i,))
@@ -290,7 +290,7 @@
         with contextlib.closing(fmap):
             current_pos = 0
             (crc32, tstamp, key_sz, value_sz, row_type,
-                 key, value, pos), new_pos = data_file.read(fmap, current_pos)
+                 key, value, pos), new_pos = data_file.read(fmap)
             current_pos = new_pos
             self.assertEqual(crc32_size+header_size+key_sz+value_sz, new_pos)
             self.assertEqual(orig_tstamp, tstamp)
@@ -299,7 +299,7 @@
             self.assertEqual('bar', value)
             self.assertEqual(0, row_type)
             (crc32, tstamp, key_sz, value_sz, row_type,
-                 key, value, pos), new_pos = data_file.read(fmap, current_pos)
+                 key, value, pos), new_pos = data_file.read(fmap)
             self.assertEqual(
                 crc32_size+header_size+key_sz+value_sz+current_pos, new_pos)
             self.assertEqual(tstamp1, tstamp)
@@ -323,7 +323,7 @@
             fd.flush()
             fmap = mmap.mmap(fd.fileno(), 0, access=mmap.ACCESS_READ)
             with contextlib.closing(fmap):
-                self.assertRaises(BadCrc, data_file.read, fmap, 0)
+                self.assertRaises(BadCrc, data_file.read, fmap)
 
     def test_read_bad_header(self):
         """Test for read method with a bad header/unpack error."""
@@ -340,7 +340,14 @@
             fd.flush()
             fmap = mmap.mmap(fd.fileno(), 0, access=mmap.ACCESS_READ)
             with contextlib.closing(fmap):
-                self.assertRaises(BadHeader, data_file.read, fmap, 0)
+                self.assertRaises(BadHeader, data_file.read, fmap)
+
+
+class NoMmapDataFileTest(DataFileTest):
+
+    def setUp(self):
+        self.patch(DataFile, 'max_mmap_size', 1)
+        return super(NoMmapDataFileTest, self).setUp()
 
 
 class TempDataFileTest(DataFileTest):
@@ -466,10 +473,9 @@
         db = Tritcask(self.base_dir)
         for i in range(10):
             db.put(i, 'foo%d' % (i,), 'bar%s' % (i,))
-        # write a different value -> random bytes
-        # now write some garbage to the end of file
-        db.live_file.fd.write(os.urandom(100))
-        db.live_file.fd.flush()
+        # write a different value to the last item.
+        db.live_file.fd.seek(db.live_file.fd.tell()-3)
+        db.live_file.fd.write(os.urandom(3))
         # and add 10 new entries
         for i in range(10, 20):
             db.put(i, 'foo%d' % (i,), 'bar%s' % (i,))
@@ -567,6 +573,25 @@
         self.assertEqual(data_file.hint_size, len("some data"))
 
 
+class NoMmapImmutableDataFileTest(ImmutableDataFileTest):
+
+    def setUp(self):
+        self.patch(ImmutableDataFile, 'max_mmap_size', 1)
+        return super(NoMmapImmutableDataFileTest, self).setUp()
+
+    def test__open(self):
+        """Test the _open private method."""
+        new_file = DataFile(self.base_dir)
+        # write some data
+        new_file.fd.write('foo')
+        immutable_file = new_file.make_immutable()
+        self.assertTrue(immutable_file.fd is not None)
+        self.assertTrue(immutable_file.fmmap is None)
+        # check that the file is opened only for read
+        self.assertRaises(IOError, immutable_file.fd.write, 'foo')
+        immutable_file.close()
+
+
 class DeadDataFileTest(ImmutableDataFileTest):
     """Tests for DeadDataFile."""
 
@@ -1277,9 +1302,10 @@
         for i in range(10):
             db.put(i, 'foo%d' % (i,), 'bar%s' % (i,))
         self.assertFalse(db.should_rotate())
-        # write a different value -> random bytes
-        # now write some garbage to the end of file
-        db.live_file.fd.write(os.urandom(100))
+        # write a different value to the last item.
+        # write a different value to the last item.
+        db.live_file.fd.seek(db.live_file.fd.tell()-5)
+        db.live_file.fd.write(os.urandom(5))
         db.live_file.fd.flush()
         db.shutdown()
         called = []
@@ -1885,3 +1911,4 @@
         """Test that the initial value is > 0."""
         timer = WindowsTimer()
         self.assertTrue(int(timer.time()) > 0)
+

=== modified file 'ubuntuone/syncdaemon/tritcask.py'
--- ubuntuone/syncdaemon/tritcask.py	2012-04-09 20:07:05 +0000
+++ ubuntuone/syncdaemon/tritcask.py	2012-05-08 14:03:17 +0000
@@ -74,10 +74,6 @@
 VERSION = 'v1'
 FILE_SUFFIX = '.tritcask-%s.data' % VERSION
 
-EXTRA_SEEK = False
-if sys.platform == 'win32':
-    EXTRA_SEEK = True
-
 logger = logging.getLogger('ubuntuone.SyncDaemon.tritcask')
 
 
@@ -182,6 +178,7 @@
     """Class that encapsulates data file handling."""
 
     last_generated_id = 0
+    max_mmap_size = int(2**31-1) # cause 2**31 it's a long in 32bits.
 
     def __init__(self, base_path, filename=None):
         """Create a DataFile instance.
@@ -250,18 +247,23 @@
             self.fd = None
 
     def iter_entries(self):
-        """Return a generator for the entries in the file."""
-        fmmap = mmap.mmap(self.fd.fileno(), 0, access=mmap.ACCESS_READ)
-        with contextlib.closing(fmmap):
-            for entry in self._iter_mmaped_entries(fmmap):
+        """Return a generator for the entries in the file using mmap."""
+        if self.size >= self.max_mmap_size:
+            for entry in self._iter_entries(self.fd):
                 yield entry
+        else:
+            fmmap = mmap.mmap(self.fd.fileno(), 0, access=mmap.ACCESS_READ)
+            with contextlib.closing(fmmap):
+                for entry in self._iter_entries(fmmap):
+                    yield entry
 
-    def _iter_mmaped_entries(self, fmmap):
+    def _iter_entries(self, fd):
         """Return a generator for the entries in the mmaped file."""
         current_pos = 0
+        fd.seek(current_pos)
         while True:
             try:
-                entry, new_pos = self.read(fmmap, current_pos)
+                entry, new_pos = self.read(fd)
                 current_pos = new_pos
                 yield entry
             except EOFError:
@@ -294,10 +296,8 @@
         tstamp = timestamp()
         header = header_struct.pack(tstamp, key_sz, value_sz, row_type)
         crc32 = crc32_struct.pack(zlib.crc32(header + key + value))
-        if EXTRA_SEEK:
-            # seek to end of file even if we are in append mode, but py2.x IO
-            # in win32 is really buggy, see: http://bugs.python.org/issue3207
-            self.fd.seek(0, os.SEEK_END)
+        # always go to the EOF before write, as we aren't using mmap any more.
+        self.fd.seek(0, os.SEEK_END)
         self.fd.write(crc32 + header)
         self.fd.write(key)
         value_pos = self.fd.tell()
@@ -305,12 +305,13 @@
         self.fd.flush()
         return tstamp, value_pos, value_sz
 
-    def read(self, fmmap, current_pos):
+    def read(self, fd):
         """Read a single entry from the current position."""
-        crc32_bytes = fmmap[current_pos:current_pos + crc32_size]
-        current_pos += crc32_size
-        header = fmmap[current_pos:current_pos + header_size]
-        current_pos += header_size
+        current_pos = fd.tell()
+        data = fd.read(crc32_size+header_size)
+        current_pos += crc32_size+header_size
+        crc32_bytes = data[:crc32_size]
+        header = data[crc32_size:]
         if header == '' or crc32_bytes == '':
             # reached EOF
             raise EOFError
@@ -319,10 +320,11 @@
             tstamp, key_sz, value_sz, row_type = header_struct.unpack(header)
         except struct.error, e:
             raise BadHeader(e)
-        key = fmmap[current_pos:current_pos + key_sz]
+        data = fd.read(key_sz+value_sz)
+        key = data[:key_sz]
         current_pos += key_sz
         value_pos = current_pos
-        value = fmmap[current_pos:current_pos + value_sz]
+        value = data[key_sz:]
         current_pos += value_sz
         # verify the crc32 of the data
         if zlib.crc32(header + key + value) == crc32:
@@ -370,8 +372,9 @@
 
     def _open(self):
         self.fd = open(self.filename, 'rb')
-        fmmap = mmap.mmap(self.fd.fileno(), 0, access=mmap.ACCESS_READ)
-        self.fmmap = fmmap
+        if self.size < self.max_mmap_size:
+            fmmap = mmap.mmap(self.fd.fileno(), 0, access=mmap.ACCESS_READ)
+            self.fmmap = fmmap
 
     def close(self):
         """Close the file descriptor and mmap."""
@@ -387,13 +390,20 @@
 
     def iter_entries(self):
         """Return a generator for the entries in the mmaped file."""
-        for entry in self._iter_mmaped_entries(self.fmmap):
-            yield entry
+        fd = self.fd
+        if self.fmmap is not None:
+            fd = self.fmmap
+        return self._iter_entries(fd)
 
     def __getitem__(self, item):
         """__getitem__ to support slicing and *only* slicing."""
         if isinstance(item, slice):
-            return self.fmmap[item]
+            # if we have an mmap, use it.
+            if self.fmmap is not None:
+                return self.fmmap[item]
+            else:
+                self.fd.seek(item.start)
+                return self.fd.read(item.stop - item.start)
         else:
             raise ValueError('Only slice is supported')