From 062b48d8d218b51484adcc35a18ec762aedb1e63 Mon Sep 17 00:00:00 2001 From: benoitc Date: Sun, 15 May 2016 02:20:13 +0200 Subject: [PATCH] lockfile improvements - rename LockFile.lock to acquire - rename LockFile.unlock to release - move the lockfile management in sepate functions inside the arbiter - remove the "closed" argument from the socket.close method and add a new "destroy" function that will be called whent the socket can be unlinked (cal release) - fix tests --- gunicorn/arbiter.py | 57 +++++++++++++++++++++++++++--------------- gunicorn/lockfile.py | 21 +++++++++------- gunicorn/sock.py | 13 +++++++--- tests/test_arbiter.py | 4 +-- tests/test_lockfile.py | 10 ++++---- tests/test_sock.py | 13 ++-------- 6 files changed, 67 insertions(+), 51 deletions(-) diff --git a/gunicorn/arbiter.py b/gunicorn/arbiter.py index b64d2a01..99a435ba 100644 --- a/gunicorn/arbiter.py +++ b/gunicorn/arbiter.py @@ -138,16 +138,7 @@ class Arbiter(object): self.LISTENERS, need_lock = create_sockets(self.cfg, self.log) if need_lock: - if not self.LOCK_FILE: - # reuse the lockfile if already set - if 'GUNICORN_LOCK' in os.environ: - lock_path = os.environ.get('GUNICORN_LOCK') - else: - lock_path = self.cfg.lockfile - - self.LOCK_FILE = LockFile(lock_path) - # add us to the shared lock - self.LOCK_FILE.lock() + self.acquire_lockfile() listeners_str = ",".join([str(l) for l in self.LISTENERS]) self.log.debug("Arbiter booted") @@ -350,17 +341,13 @@ class Arbiter(object): :attr graceful: boolean, If True (the default) workers will be killed gracefully (ie. trying to wait for the current connection) """ - locked = False - if self.LOCK_FILE: - self.LOCK_FILE.unlock() - locked = self.LOCK_FILE.locked() - - # delete the lock file if needed - if not locked and 'GUNICORN_LOCK' in os.environ: - del os.environ['GUNICORN_LOCK'] - + released = self.try_release_lockfile() for l in self.LISTENERS: - l.close(locked) + l.close() + + if released: + l.destroy() + self.LISTENERS = [] sig = signal.SIGTERM if not graceful: @@ -453,6 +440,36 @@ class Arbiter(object): # manage workers self.manage_workers() + def acquire_lockfile(self): + """\ + Acquire the lock file + """ + if not self.LOCK_FILE: + # reuse the lockfile if already set + if 'GUNICORN_LOCK' in os.environ: + lock_path = os.environ.get('GUNICORN_LOCK') + else: + lock_path = self.cfg.lockfile + + self.LOCK_FILE = LockFile(lock_path) + # add us to the shared lock + self.LOCK_FILE.acquire() + + def try_release_lockfile(self): + """\ + Try to release the lock file + """ + released = False + if self.LOCK_FILE: + released = self.LOCK_FILE.release() + + # delete the lock file if needed + if released and 'GUNICORN_LOCK' in os.environ: + del os.environ['GUNICORN_LOCK'] + + return released + + def murder_workers(self): """\ Kill unused/idle workers diff --git a/gunicorn/lockfile.py b/gunicorn/lockfile.py index 1815611a..6b841d9d 100644 --- a/gunicorn/lockfile.py +++ b/gunicorn/lockfile.py @@ -49,24 +49,27 @@ class LockFile(object): self._lockfile = open(self.fname, 'w+b') # set permissions to -rw-r--r-- os.chmod(self.fname, 420) - self._locked = False + self._released = True - def lock(self): + def acquire(self): _lock(self._lockfile.fileno()) - self._locked = True + self._released = False - def unlock(self): - if not self.locked(): - return + def release(self): + if self.released(): + return True if _unlock(self._lockfile.fileno()): self._lockfile.close() util.unlink(self.fname) self._lockfile = None - self._locked = False + self._released = True + return True - def locked(self): - return self._lockfile is not None and self._locked + return False + + def released(self): + return self._lockfile is None or self._released def name(self): return self.fname diff --git a/gunicorn/sock.py b/gunicorn/sock.py index 341a48c3..67d20671 100644 --- a/gunicorn/sock.py +++ b/gunicorn/sock.py @@ -53,7 +53,7 @@ class BaseSocket(object): def bind(self, sock): sock.bind(self.cfg_addr) - def close(self, locked=False): + def close(self): if self.sock is None: return @@ -64,6 +64,9 @@ class BaseSocket(object): self.sock = None + def destroy(self): + pass + class TCPSocket(BaseSocket): @@ -120,11 +123,13 @@ class UnixSocket(BaseSocket): util.chown(self.cfg_addr, self.conf.uid, self.conf.gid) os.umask(old_umask) - def close(self, locked=False): - if self.parent == os.getpid() and not locked: - os.unlink(self.cfg_addr) + def close(self): super(UnixSocket, self).close() + def destroy(self): + if self.parent == os.getpid(): + os.unlink(self.cfg_addr) + def _sock_type(addr): if isinstance(addr, tuple): diff --git a/tests/test_arbiter.py b/tests/test_arbiter.py index 277ae768..d6e06dfd 100644 --- a/tests/test_arbiter.py +++ b/tests/test_arbiter.py @@ -35,8 +35,8 @@ def test_arbiter_shutdown_closes_listeners(): listener2 = mock.Mock() arbiter.LISTENERS = [listener1, listener2] arbiter.stop() - listener1.close.assert_called_with(False) - listener2.close.assert_called_with(False) + listener1.close.assert_called_with() + listener2.close.assert_called_with() class PreloadedAppWithEnvSettings(DummyApplication): diff --git a/tests/test_lockfile.py b/tests/test_lockfile.py index 1e4d683d..9ca485cf 100644 --- a/tests/test_lockfile.py +++ b/tests/test_lockfile.py @@ -6,10 +6,10 @@ from gunicorn.util import tmpfile def test_lockfile(): lockname = tmpfile(prefix="gunicorn-tests", suffix=".lock") lock_file = LockFile(lockname) - assert lock_file.locked() == False + assert lock_file.released() == True assert os.path.exists(lockname) - lock_file.lock() - assert lock_file.locked() == True - lock_file.unlock() - assert lock_file.locked() == False + lock_file.acquire() + assert lock_file.released() == False + lock_file.release() + assert lock_file.released() == True assert os.path.exists(lockname) == False diff --git a/tests/test_sock.py b/tests/test_sock.py index 92c2aeb0..0782bf7d 100644 --- a/tests/test_sock.py +++ b/tests/test_sock.py @@ -11,19 +11,10 @@ from gunicorn import sock @mock.patch('socket.fromfd') def test_unix_socket_close_delete_if_exlock(fromfd, unlink, getpid): gsock = sock.UnixSocket('test.sock', mock.Mock(), mock.Mock(), mock.Mock()) - gsock.close(False) + gsock.destroy() unlink.assert_called_with('test.sock') -@mock.patch('os.getpid') -@mock.patch('os.unlink') -@mock.patch('socket.fromfd') -def test_unix_socket_close_keep_if_no_exlock(fromfd, unlink, getpid): - gsock = sock.UnixSocket('test.sock', mock.Mock(), mock.Mock(), mock.Mock()) - gsock.close(True) - unlink.assert_not_called() - - @mock.patch('os.getpid') @mock.patch('os.unlink') @mock.patch('socket.fromfd') @@ -32,5 +23,5 @@ def test_unix_socket_not_deleted_by_worker(fromfd, unlink, getpid): gsock = sock.UnixSocket('test.sock', mock.Mock(), mock.Mock(), fd) getpid.reset_mock() getpid.return_value = "fake" # fake a pid change - gsock.close(False) + gsock.destroy() unlink.assert_not_called()