Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RHELC-1124] Refactor applock to have a more robust API. #979

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
121 changes: 85 additions & 36 deletions convert2rhel/applock.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import errno
import logging
import os
import stat
import tempfile


Expand Down Expand Up @@ -59,15 +60,16 @@
def __init__(self, name):
# Our application name
self._name = name
# Do we think we locked the pid file?
self._locked = False
# Our process ID
# Our process ID. We save this when the lock is created so it will be
# consistent even if we check from inside a fork.
self._pid = os.getpid()
# Path to the file that contains the process id
self._pidfile = os.path.join(_DEFAULT_LOCK_DIR, self._name + ".pid")
# File object for _pidfile when the file is locked
self._pidfile_fp = None

def __str__(self):
if self._locked:
if self.is_locked:
status = "locked"
else:
status = "unlocked"
Expand All @@ -89,6 +91,7 @@
# Note that NamedTemporaryFile will clean up the file it created,
# but the lockfile we created by doing the link will stay around.
with tempfile.NamedTemporaryFile(mode="w", suffix=".pid", prefix=self._name, dir=_DEFAULT_LOCK_DIR) as f:
os.chmod(f.name, stat.S_IRUSR | stat.S_IWUSR)
f.write(str(self._pid) + "\n")
f.flush()
try:
Expand All @@ -97,14 +100,53 @@
except OSError as exc:
if exc.errno == errno.EEXIST:
return False
raise exc
Venefilyn marked this conversation as resolved.
Show resolved Hide resolved
raise

Check warning on line 103 in convert2rhel/applock.py

View check run for this annotation

Codecov / codecov/patch

convert2rhel/applock.py#L103

Added line #L103 was not covered by tests
loggerinst.debug("%s." % self)
return True

r0x0d marked this conversation as resolved.
Show resolved Hide resolved
@property
def is_locked(self):
"""Test whether this object is locked."""
return self._locked
"""Test whether this object was locked by this instance of
the application."""
pid = self._read_pidfile()
self._release_pidfile_flock()
if pid is not None:
r0x0d marked this conversation as resolved.
Show resolved Hide resolved
return pid == self._pid
return False

def _release_pidfile_flock(self):
"""Release the advisory file lock we hold on the PID file
and close the open file descriptor."""
if self._pidfile_fp is not None:
r0x0d marked this conversation as resolved.
Show resolved Hide resolved
if not self._pidfile_fp.closed:
self._pidfile_fp.close()
self._pidfile_fp = None

def _read_pidfile(self):
"""Read and return the contents of the PID file.

If this method does not raise an exception, the
_release_pidfile_flock() method must be called to release the
advisory lock and close the file.

:returns: the file contents as an integer, or None if it doesn't exist
:raises: ApplicationLockedError if the contents are corrupt
"""
# pylint: disable=consider-using-with
try:
self._pidfile_fp = open(self._pidfile, "r")
Fixed Show fixed Hide fixed
github-advanced-security[bot] marked this conversation as resolved.
Fixed
Show resolved Hide resolved
file_contents = self._pidfile_fp.read()
pid = int(file_contents.rstrip())
return pid
except IOError as exc:
# The open() failed because the file exists.
# In Python 3 this could be changed to FileNotFoundError.
if exc.errno == errno.ENOENT:
return None
raise
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, we should be raising exception again

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have to leave the file open and close it later explicitly because the file lock is tied to the file descriptor. The file lock is required to plug a race condition in which the pid file is overwritten by another process between the point where we read its contents and unlink it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bare raise statement reraises the current exception; @abadger requested that I use this idiom.

except ValueError:
self._release_pidfile_flock()
raise ApplicationLockedError("Lock file %s is corrupt" % self._pidfile)

@staticmethod
def _pid_exists(pid):
Expand All @@ -120,52 +162,59 @@
return False
return True

def try_to_lock(self, _recursive=False):
"""Try to get a lock on this application. If successful,
the application will be locked; the lock should be released
with unlock().
def _safe_unlink(self):
"""Unlink the lock file. If the unlink fails because the file
doesn't exist, swallow the exception; this avoids spurious
errors due to race conditions.
"""
try:
os.unlink(self._pidfile)
except OSError as exc:

Check warning on line 172 in convert2rhel/applock.py

View check run for this annotation

Codecov / codecov/patch

convert2rhel/applock.py#L172

Added line #L172 was not covered by tests
# In Python 3 this could be changed to FileNotFoundError.
if exc.errno == errno.ENOENT:
return
raise

Check warning on line 176 in convert2rhel/applock.py

View check run for this annotation

Codecov / codecov/patch

convert2rhel/applock.py#L175-L176

Added lines #L175 - L176 were not covered by tests
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't covered by tests


def try_to_lock(self):
"""Try to get a lock on this application. If this method does
not raise an Exception, the application will be locked and we
hold the lock; the lock should be released with unlock().

If the file has unexpected contents, for safety we treat the
application as locked, since it is probably the result of
manual meddling, intentional or otherwise.

:keyword _recursive: True if we are being called recursively
and should not try to clean up the lockfile
again.
:raises ApplicationLockedError: the application is locked
"""
if self._try_create():
self._locked = True
return
if _recursive:
raise ApplicationLockedError("Cannot lock %s" % self._name)

with open(self._pidfile, "r") as f:
file_contents = f.read()
try:
pid = int(file_contents.rstrip())
except ValueError:
raise ApplicationLockedError("Lock file %s is corrupt" % self._pidfile)

if self._pid_exists(pid):
raise ApplicationLockedError("%s locked by process %d" % (self._pidfile, pid))
# The lock file was created by a process that has exited;
# remove it and try again.
loggerinst.info("Cleaning up lock held by exited process %d." % pid)
os.unlink(self._pidfile)
self.try_to_lock(_recursive=True)
pid = self._read_pidfile()
if pid == self._pid:
return
if self._pid_exists(pid):
raise ApplicationLockedError("%s locked by process %d" % (self._pidfile, pid))
# The lock file was created by a process that has exited;
# remove it and try again.
loggerinst.info("Cleaning up lock held by exited process %d." % pid)
self._safe_unlink()
finally:
self._release_pidfile_flock()
if not self._try_create():
# Between the unlink and our attempt to create the lock
# file, another process got there first.
raise ApplicationLockedError("%s is locked" % self._pidfile)

Check warning on line 206 in convert2rhel/applock.py

View check run for this annotation

Codecov / codecov/patch

convert2rhel/applock.py#L206

Added line #L206 was not covered by tests

def unlock(self):
"""Release the lock on this application.

Note that if the unlink fails (a pathological failure) the
object will stay locked and the OSError or other
Note that if the safe unlink fails (a pathological failure)
the object will stay locked and the OSError or other
system-generated exception will be raised.
"""
if not self._locked:
if not self.is_locked:
return
os.unlink(self._pidfile)
self._locked = False
self._safe_unlink()
loggerinst.debug("%s." % self)

def __enter__(self):
Expand Down
13 changes: 11 additions & 2 deletions convert2rhel/unit_tests/applock_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,13 +51,22 @@ def test_applock_basic(tmp_lock):

def test_applock_basic_islocked(tmp_lock):
with open(tmp_lock._pidfile, "w") as f:
pid = os.getpid()
f.write(str(pid) + "\n")
# Our parent process will be running and have a different pid
ppid = os.getppid()
f.write(str(ppid) + "\n")
with pytest.raises(applock.ApplicationLockedError):
tmp_lock.try_to_lock()
os.unlink(tmp_lock._pidfile)


def test_applock_basic_lock_idempotent(tmp_lock):
with open(tmp_lock._pidfile, "w") as f:
pid = os.getpid()
f.write(str(pid) + "\n")
tmp_lock.try_to_lock()
os.unlink(tmp_lock._pidfile)


def test_applock_basic_reap(tmp_lock):
"""Test the case where the lockfile was held by a process
that has exited."""
Expand Down
Loading