diff --git a/swiftonfile/swift/obj/diskfile.py b/swiftonfile/swift/obj/diskfile.py index bbbf913..b8e950a 100644 --- a/swiftonfile/swift/obj/diskfile.py +++ b/swiftonfile/swift/obj/diskfile.py @@ -615,37 +615,49 @@ class DiskFile(object): """ # Writes are always performed to a temporary file try: - fd = do_open(self._data_file, os.O_RDONLY | O_CLOEXEC) + self._fd = do_open(self._data_file, os.O_RDONLY | O_CLOEXEC) except SwiftOnFileSystemOSError as err: if err.errno in (errno.ENOENT, errno.ENOTDIR): # If the file does exist, or some part of the path does not # exist, raise the expected DiskFileNotExist raise DiskFileNotExist raise - else: - stats = do_fstat(fd) - if not stats: - return + try: + stats = do_fstat(self._fd) self._is_dir = stat.S_ISDIR(stats.st_mode) obj_size = stats.st_size - self._metadata = read_metadata(fd) - if not validate_object(self._metadata): - create_object_metadata(fd) - self._metadata = read_metadata(fd) - assert self._metadata is not None - self._filter_metadata() + self._metadata = read_metadata(self._fd) + if not validate_object(self._metadata): + create_object_metadata(self._fd) + self._metadata = read_metadata(self._fd) + assert self._metadata is not None + self._filter_metadata() - if self._is_dir: - do_close(fd) - obj_size = 0 - self._fd = -1 - else: - if self._is_object_expired(self._metadata): - raise DiskFileExpired(metadata=self._metadata) - self._fd = fd + if self._is_dir: + do_close(self._fd) + obj_size = 0 + self._fd = -1 + else: + if self._is_object_expired(self._metadata): + raise DiskFileExpired(metadata=self._metadata) + self._obj_size = obj_size + except (OSError, IOError, DiskFileExpired) as err: + # Something went wrong. Context manager will not call + # __exit__. So we close the fd manually here. + self._close_fd() + if hasattr(err, 'errno') and err.errno == errno.ENOENT: + # Handle races: ENOENT can be raised by read_metadata() + # call in GlusterFS if file gets deleted by another + # client after do_open() succeeds + logging.warn("open(%s) succeeded but one of the subsequent " + "syscalls failed with ENOENT. Raising " + "DiskFileNotExist." % (self._data_file)) + raise DiskFileNotExist + else: + # Re-raise the original exception after fd has been closed + raise - self._obj_size = obj_size return self def _is_object_expired(self, metadata): @@ -683,6 +695,12 @@ class DiskFile(object): raise DiskFileNotOpen() return self + def _close_fd(self): + if self._fd is not None: + fd, self._fd = self._fd, None + if fd > -1: + do_close(fd) + def __exit__(self, t, v, tb): """ Context exit. @@ -694,10 +712,7 @@ class DiskFile(object): responsibility of the implementation to properly handle that. """ self._metadata = None - if self._fd is not None: - fd, self._fd = self._fd, None - if fd > -1: - do_close(fd) + self._close_fd() def get_metadata(self): """ diff --git a/test/unit/obj/test_diskfile.py b/test/unit/obj/test_diskfile.py index c71c889..0bc0d73 100644 --- a/test/unit/obj/test_diskfile.py +++ b/test/unit/obj/test_diskfile.py @@ -26,18 +26,19 @@ from eventlet import tpool from mock import Mock, patch from hashlib import md5 from copy import deepcopy +from contextlib import nested from swiftonfile.swift.common.exceptions import AlreadyExistsAsDir, \ AlreadyExistsAsFile -from swift.common.exceptions import DiskFileNotExist, DiskFileError, \ - DiskFileNoSpace, DiskFileNotOpen +from swift.common.exceptions import DiskFileNoSpace, DiskFileNotOpen, \ + DiskFileNotExist, DiskFileExpired from swift.common.utils import ThreadPool from swiftonfile.swift.common.exceptions import SwiftOnFileSystemOSError import swiftonfile.swift.common.utils from swiftonfile.swift.common.utils import normalize_timestamp import swiftonfile.swift.obj.diskfile -from swiftonfile.swift.obj.diskfile import DiskFileWriter, DiskFile, DiskFileManager -from swiftonfile.swift.common.utils import DEFAULT_UID, DEFAULT_GID, X_TYPE, \ +from swiftonfile.swift.obj.diskfile import DiskFileWriter, DiskFileManager +from swiftonfile.swift.common.utils import DEFAULT_UID, DEFAULT_GID, \ X_OBJECT_TYPE, DIR_OBJECT from test.unit.common.test_utils import _initxattr, _destroyxattr @@ -972,3 +973,70 @@ class TestDiskFile(unittest.TestCase): assert os.path.exists(gdf._data_file) # Real file exists assert not os.path.exists(tmppath) # Temp file does not exist + + def test_fd_closed_when_diskfile_open_raises_exception_race(self): + # do_open() succeeds but read_metadata() fails(GlusterFS) + _m_do_open = Mock(return_value=999) + _m_do_fstat = Mock(return_value= + os.stat_result((33261, 2753735, 2053, 1, 1000, + 1000, 6873, 1431415969, + 1376895818, 1433139196))) + _m_rmd = Mock(side_effect=IOError(errno.ENOENT, + os.strerror(errno.ENOENT))) + _m_do_close = Mock() + _m_log = Mock() + + with nested( + patch("swiftonfile.swift.obj.diskfile.do_open", _m_do_open), + patch("swiftonfile.swift.obj.diskfile.do_fstat", _m_do_fstat), + patch("swiftonfile.swift.obj.diskfile.read_metadata", _m_rmd), + patch("swiftonfile.swift.obj.diskfile.do_close", _m_do_close), + patch("swiftonfile.swift.obj.diskfile.logging.warn", _m_log)): + gdf = self._get_diskfile("vol0", "p57", "ufo47", "bar", "z") + try: + with gdf.open(): + pass + except DiskFileNotExist: + pass + else: + self.fail("Expecting DiskFileNotExist") + _m_do_fstat.assert_called_once_with(999) + _m_rmd.assert_called_once_with(999) + _m_do_close.assert_called_once_with(999) + self.assertFalse(gdf._fd) + # Make sure ENOENT failure is logged + self.assertTrue("failed with ENOENT" in _m_log.call_args[0][0]) + + def test_fd_closed_when_diskfile_open_raises_DiskFileExpired(self): + # A GET/DELETE on an expired object should close fd + the_path = os.path.join(self.td, "vol0", "ufo47", "bar") + the_file = os.path.join(the_path, "z") + os.makedirs(the_path) + with open(the_file, "w") as fd: + fd.write("1234") + md = { + 'X-Type': 'Object', + 'X-Object-Type': 'file', + 'Content-Length': str(os.path.getsize(the_file)), + 'ETag': md5("1234").hexdigest(), + 'X-Timestamp': os.stat(the_file).st_mtime, + 'X-Delete-At': 0, # This is in the past + 'Content-Type': 'application/octet-stream'} + _metadata[_mapit(the_file)] = md + + _m_do_close = Mock() + + with patch("swiftonfile.swift.obj.diskfile.do_close", _m_do_close): + gdf = self._get_diskfile("vol0", "p57", "ufo47", "bar", "z") + try: + with gdf.open(): + pass + except DiskFileExpired: + # Confirm that original exception is re-raised + pass + else: + self.fail("Expecting DiskFileExpired") + self.assertEqual(_m_do_close.call_count, 1) + self.assertFalse(gdf._fd) + # Close the actual fd, as we had mocked do_close + os.close(_m_do_close.call_args[0][0])