Avoid unnecessary unlink() on every successful PUT

If you do a strace on object-server PUT operation, you'd see that
there's an unlink() sys call which _always_ fails with ENOENT.

mkstemp() creates a temp file which is renamed later to it's final
object path in filesystem. Hence, on a successful object PUT, the
tempfile would never exist in its original location after rename.

Change-Id: I805c7c200107e2d56278f0fb35692a51cb1edc0b
Signed-off-by: Prashanth Pai <ppai@redhat.com>

swift/obj/diskfile.py

@@ -113,7 +113,7 @@ def read_metadata(fd):
     try:
         while True:
             metadata += xattr.getxattr(fd, '%s%s' % (METADATA_KEY,
-                                                    (key or '')))
+                                                     (key or '')))
             key += 1
     except IOError as e:
         for err in 'ENOTSUP', 'EOPNOTSUPP':
@@ -806,6 +806,11 @@ class DiskFileWriter(object):
         self._upload_size = 0
         self._last_sync = 0
         self._extension = '.data'
+        self._put_succeeded = False
+
+    @property
+    def put_succeeded(self):
+        return self._put_succeeded
 
     def write(self, chunk):
         """
@@ -852,6 +857,10 @@ class DiskFileWriter(object):
         # After the rename completes, this object will be available for other
         # requests to reference.
         renamer(self._tmppath, target_path)
+        # If rename is successful, flag put as succeeded. This is done to avoid
+        # unnecessary os.unlink() of tempfile later. As renamer() has
+        # succeeded, the tempfile would no longer exist at its original path.
+        self._put_succeeded = True
         try:
             hash_cleanup_listdir(self._datadir)
         except OSError:
@@ -1595,23 +1604,31 @@ class DiskFile(object):
         if not exists(self._tmpdir):
             mkdirs(self._tmpdir)
         fd, tmppath = mkstemp(dir=self._tmpdir)
+        dfw = None
         try:
             if size is not None and size > 0:
                 try:
                     fallocate(fd, size)
                 except OSError:
                     raise DiskFileNoSpace()
-            yield DiskFileWriter(self._name, self._datadir, fd, tmppath,
+            dfw = DiskFileWriter(self._name, self._datadir, fd, tmppath,
                                  self._bytes_per_sync, self._threadpool)
+            yield dfw
         finally:
             try:
                 os.close(fd)
             except OSError:
                 pass
-            try:
-                os.unlink(tmppath)
-            except OSError:
-                pass
+            if (dfw is None) or (not dfw.put_succeeded):
+                # Try removing the temp file only if put did NOT succeed.
+                #
+                # dfw.put_succeeded is set to True after renamer() succeeds in
+                # DiskFileWriter._finalize_put()
+                try:
+                    os.unlink(tmppath)
+                except OSError:
+                    self._logger.exception('Error removing tempfile: %s' %
+                                           tmppath)
 
     def write_metadata(self, metadata):
         """

test/unit/obj/test_diskfile.py

@@ -2370,5 +2370,74 @@ class TestDiskFile(unittest.TestCase):
             else:
                 self.fail('`splice` not called with expected arguments')
 
+    def test_create_unlink_cleanup_DiskFileNoSpace(self):
+        # Test cleanup when DiskFileNoSpace() is raised.
+        df = self.df_mgr.get_diskfile(self.existing_device, '0', 'abc', '123',
+                                      'xyz')
+        _m_fallocate = mock.MagicMock(side_effect=OSError(errno.ENOSPC,
+                                      os.strerror(errno.ENOSPC)))
+        _m_unlink = mock.Mock()
+        with mock.patch("swift.obj.diskfile.fallocate", _m_fallocate):
+            with mock.patch("os.unlink", _m_unlink):
+                try:
+                    with df.create(size=100):
+                        pass
+                except DiskFileNoSpace:
+                    pass
+                else:
+                    self.fail("Expected exception DiskFileNoSpace")
+        self.assertTrue(_m_fallocate.called)
+        self.assertTrue(_m_unlink.called)
+        self.assert_(len(self.df_mgr.logger.log_dict['exception']) == 0)
+
+    def test_create_unlink_cleanup_renamer_fails(self):
+        # Test cleanup when renamer fails
+        _m_renamer = mock.MagicMock(side_effect=OSError(errno.ENOENT,
+                                    os.strerror(errno.ENOENT)))
+        _m_unlink = mock.Mock()
+        df = self._simple_get_diskfile()
+        data = '0' * 100
+        metadata = {
+            'ETag': md5(data).hexdigest(),
+            'X-Timestamp': Timestamp(time()).internal,
+            'Content-Length': str(100),
+        }
+        with mock.patch("swift.obj.diskfile.renamer", _m_renamer):
+            with mock.patch("os.unlink", _m_unlink):
+                try:
+                    with df.create(size=100) as writer:
+                        writer.write(data)
+                        writer.put(metadata)
+                except OSError:
+                    pass
+                else:
+                    self.fail("Expected OSError exception")
+        self.assertFalse(writer.put_succeeded)
+        self.assertTrue(_m_renamer.called)
+        self.assertTrue(_m_unlink.called)
+        self.assert_(len(self.df_mgr.logger.log_dict['exception']) == 0)
+
+    def test_create_unlink_cleanup_logging(self):
+        # Test logging of os.unlink() failures.
+        df = self.df_mgr.get_diskfile(self.existing_device, '0', 'abc', '123',
+                                      'xyz')
+        _m_fallocate = mock.MagicMock(side_effect=OSError(errno.ENOSPC,
+                                      os.strerror(errno.ENOSPC)))
+        _m_unlink = mock.MagicMock(side_effect=OSError(errno.ENOENT,
+                                   os.strerror(errno.ENOENT)))
+        with mock.patch("swift.obj.diskfile.fallocate", _m_fallocate):
+            with mock.patch("os.unlink", _m_unlink):
+                try:
+                    with df.create(size=100):
+                        pass
+                except DiskFileNoSpace:
+                    pass
+                else:
+                    self.fail("Expected exception DiskFileNoSpace")
+        self.assertTrue(_m_fallocate.called)
+        self.assertTrue(_m_unlink.called)
+        self.assert_(self.df_mgr.logger.log_dict['exception'][0][0][0].
+                     startswith("Error removing tempfile:"))
+
 if __name__ == '__main__':
     unittest.main()