Fix double close issue in diskfile

The DiskWriter was closing the file descriptor when it finished
writing but initializing it to None.  Therefore, the DiskFile
context manager would then try to close it also.

Change-Id: I188ec814d025e28c7b89532f0502ebf1d4a20a09
Signed-off-by: Luis Pabon <lpabon@redhat.com>
Reviewed-on: http://review.gluster.org/6317
Reviewed-by: Peter Portante <pportant@redhat.com>
Reviewed-by: pushpesh sharma <psharma@redhat.com>
Tested-by: pushpesh sharma <psharma@redhat.com>
Signed-off-by: Luis Pabon <lpabon@redhat.com>
Reviewed-on: http://review.gluster.org/6487

2 files changed, 35 insertions, 15 deletions

diff --git a/gluster/swift/obj/diskfile.py b/gluster/swift/obj/diskfile.py
index cccd5e6..3e4c5af 100644
--- a/gluster/swift/obj/diskfile.py
+++ b/gluster/swift/obj/diskfile.py
@@ -367,6 +367,14 @@ class DiskFileWriter(object):
                 do_fadvise64(self._fd, self._last_sync, diff)
                 self._last_sync = self._upload_size
 
+    def close(self):
+        """
+        Close the file descriptor
+        """
+        if self._fd:
+            do_close(self._fd)
+            self._fd = None
+
     def write(self, chunk):
         """
         Write a chunk of data to disk.
@@ -463,10 +471,9 @@ class DiskFileWriter(object):
             else:
                 # Success!
                 break
-        # Close here so the calling context does not have to perform this,
-        # which keeps all file system operations in the threadpool context.
-        do_close(self._fd)
-        self._fd = None
+        # Close here so the calling context does not have to perform this
+        # in a thread.
+        self.close()
 
     def put(self, metadata):
         """
@@ -966,14 +973,9 @@ class DiskFile(object):
             dw = DiskFileWriter(fd, tmppath, self)
             yield dw
         finally:
-            if dw is not None:
-                try:
-                    if dw._fd:
-                        do_close(dw._fd)
-                except OSError:
-                    pass
-                if dw._tmppath:
-                    do_unlink(dw._tmppath)
+            dw.close()
+            if dw._tmppath:
+                do_unlink(dw._tmppath)
 
     def write_metadata(self, metadata):
         """
diff --git a/test/unit/obj/test_diskfile.py b/test/unit/obj/test_diskfile.py
index 340087f..ecec402 100644
--- a/test/unit/obj/test_diskfile.py
+++ b/test/unit/obj/test_diskfile.py
@@ -23,7 +23,7 @@ import tempfile
 import shutil
 import mock
 from eventlet import tpool
-from mock import patch
+from mock import Mock, patch
 from hashlib import md5
 from copy import deepcopy
 
@@ -35,7 +35,7 @@ from gluster.swift.common.exceptions import GlusterFileSystemOSError
 import gluster.swift.common.utils
 from gluster.swift.common.utils import normalize_timestamp
 import gluster.swift.obj.diskfile
-from gluster.swift.obj.diskfile import DiskFile, OnDiskManager
+from gluster.swift.obj.diskfile import DiskFileWriter, DiskFile, OnDiskManager
 from gluster.swift.common.utils import DEFAULT_UID, DEFAULT_GID, X_TYPE, \
     X_OBJECT_TYPE, DIR_OBJECT
 
@@ -94,6 +94,24 @@ class MockRenamerCalled(Exception):
 def _mock_renamer(a, b):
     raise MockRenamerCalled()
 
+class TestDiskFileWriter(unittest.TestCase):
+    """ Tests for gluster.swift.obj.diskfile.DiskFileWriter """
+
+    def test_close(self):
+        dw = DiskFileWriter(100, 'a', None)
+        mock_close = Mock()
+        with patch("gluster.swift.obj.diskfile.do_close", mock_close):
+            # It should call do_close
+            self.assertEqual(100, dw._fd)
+            dw.close()
+            self.assertEqual(1, mock_close.call_count)
+            self.assertEqual(None, dw._fd)
+
+            # It should not call do_close since it should
+            # have made fd equal to None
+            dw.close()
+            self.assertEqual(None, dw._fd)
+            self.assertEqual(1, mock_close.call_count)
 
 class TestDiskFile(unittest.TestCase):
     """ Tests for gluster.swift.obj.diskfile """
@@ -881,7 +899,7 @@ class TestDiskFile(unittest.TestCase):
             assert os.path.exists(saved_tmppath)
             dw.write("123")
             # Closing the fd prematurely should not raise any exceptions.
-            os.close(dw._fd)
+            dw.close()
         assert not os.path.exists(saved_tmppath)
 
     def test_create_err_on_unlink(self):