Merge "DiskFile(Writer) refactor cleanups"

swift/obj/diskfile.py

@@ -1642,11 +1642,7 @@ class BaseDiskFileWriter(object):
 
     @property
     def logger(self):
-        try:
-            return self._logger
-        except AttributeError:
-            self._logger = self.manager.logger
-        return self._logger
+        return self.manager.logger
 
     def _get_tempfile(self):
         fallback_to_mkstemp = False
@@ -1673,6 +1669,9 @@ class BaseDiskFileWriter(object):
         return fd, tmppath
 
     def open(self):
+        if self._fd is not None:
+            raise ValueError('DiskFileWriter is already open')
+
         try:
             self._fd, self._tmppath = self._get_tempfile()
         except OSError as err:
@@ -1695,6 +1694,7 @@ class BaseDiskFileWriter(object):
                 os.close(self._fd)
             except OSError:
                 pass
+            self._fd = None
         if self._tmppath and not self._put_succeeded:
             # Try removing the temp file only if put did NOT succeed.
             #
@@ -1706,6 +1706,7 @@ class BaseDiskFileWriter(object):
             except OSError:
                 self.logger.exception('Error removing tempfile: %s' %
                                       self._tmppath)
+            self._tmppath = None
 
     def write(self, chunk):
         """
@@ -1719,6 +1720,9 @@ class BaseDiskFileWriter(object):
         :returns: the total number of bytes written to an object
         """
 
+        if not self._fd:
+            raise ValueError('Writer is not open')
+
         while chunk:
             written = os.write(self._fd, chunk)
             self._upload_size += written

swift/obj/mem_diskfile.py

@@ -97,7 +97,6 @@ class DiskFileWriter(object):
 
     :param fs: internal file system object to use
     :param name: standard object name
-    :param fp: `StringIO` in-memory representation object
     """
     def __init__(self, fs, name):
         self._filesystem = fs
@@ -106,10 +105,21 @@ class DiskFileWriter(object):
         self._upload_size = 0
 
     def open(self):
+        """
+        Prepare to accept writes.
+
+        Create a new ``StringIO`` object for a started-but-not-yet-finished
+        PUT.
+        """
         self._fp = moves.cStringIO()
         return self
 
     def close(self):
+        """
+        Clean up resources following an ``open()``.
+
+        Note: If ``put()`` has not been called, the data written will be lost.
+        """
         self._fp = None
 
     def write(self, chunk):

test/unit/obj/test_diskfile.py

@@ -3537,6 +3537,23 @@ class DiskFileMixin(BaseDiskFileTestMixin):
             with df.create():
                 self.assertTrue(os.path.exists(tmpdir))
 
+    def test_disk_file_writer(self):
+        df = self._simple_get_diskfile()
+        with df.create() as writer:
+            self.assertIsInstance(writer, diskfile.BaseDiskFileWriter)
+            # create automatically opens for us
+            self.assertIsNotNone(writer._fd)
+            # can't re-open since we're already open
+            with self.assertRaises(ValueError):
+                writer.open()
+            writer.write(b'asdf')
+            writer.close()
+            # can't write any more
+            with self.assertRaises(ValueError):
+                writer.write(b'asdf')
+            # can close again
+            writer.close()
+
     def _get_open_disk_file(self, invalid_type=None, obj_name='o', fsize=1024,
                             csize=8, mark_deleted=False, prealloc=False,
                             ts=None, mount_check=False, extra_metadata=None,