From a55740493e2acf6aeed00a0e7779deea89ee5097 Mon Sep 17 00:00:00 2001 From: Prashanth Pai Date: Tue, 29 Sep 2015 12:44:51 +0530 Subject: [PATCH] Fix object server leaking file descriptors This is a manual forward-port if the following change in icehouse branch: https://review.openstack.org/211866 Object server never closes file descriptor when DiskFile.open() raises an exception. This is fixed by catching exceptions thrown by code block inside DiskFile.open() and manually closing the file descriptor. Change-Id: Ie3e047443f4893e21b9cbdea691867f069363d7e Signed-off-by: Prashanth Pai --- swiftonfile/swift/obj/diskfile.py | 63 +++++++++++++++---------- test/unit/obj/test_diskfile.py | 76 +++++++++++++++++++++++++++++-- 2 files changed, 111 insertions(+), 28 deletions(-) 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])