LocalFileIdentifiableStore: Fix concurrency issues - #591
Conversation
Previously the LocalFileIdentifiableStore allowed multiple instances (potentially across multiple processes) to acces the same directory. This can lead to undetected invalid cache entries. Additionally it would increase the overhead for adding thread-safety to the R/W operations as synchronization across multiple instances would be necessary. In order to overcome these limitations, these changes introduce a directory lock mechanism, that only allows one instance of the class to hold a lock on a `.lock` file in the directory. A contex manager is used to secure operations on the directory and prevent concurrent release of the lock. For now this only work on POSIX machines, as it relies on `flock` functionatily which is not provided by Windows.
Until now the `LocalFileIdentifiableStore` had a TOCTOU race condition
in the `add()` and `commit()` methods. After a file was checked
for existance a concurrent running `add()` or `discard()` could still
create or remove the file before the original invocation accesses it.
To fix this, all write accesses to a file need to be mutual exclusive.
In this implementation I chose to use a global `_writing_lock` to
lock write operations to all files of the store, as locking individual
files has no performance benefit, due to serialization of multiple threads
through Pythons GIL.
This implementation prevents the following concurrency issues:
1. Two concurrent `add()`: Instead of silent overwriting, first call will
succeed, second failes with `KeyError`.
2. `commit()` + `discard()`: Instead of `commit()` creating file after
`discard()` deleted it, the file is now certainly discarded.
|
@hpoeche could you update this branch with the changes from |
|
I resolved all merge conflicts and made sure |
s-heppner
left a comment
s-heppner
left a comment
There was a problem hiding this comment.
The DescriptorStore in server/app/backend/local_file.py needs the same mechanism. I'm fine with moving this into a separate issue.
| .. warning:: | ||
| This backend is intended for development and testing only. It provides no | ||
| concurrency control across processes: concurrent writes to the same object | ||
| (e.g. under a multi-worker WSGI server) will silently overwrite each other, | ||
| with the last writer winning and no error raised. Use a dedicated database | ||
| backend for any production deployment. |
There was a problem hiding this comment.
This still holds true for Windows, as on Windows the whole locking mechanism is simply no-op right? Maybe we should keep and adapt the warning? Or maybe even add a flag that a Windows user needs to set in order to acknowledge that the store is not concurrency safe?
| # We need to prevent multiple instances of LocalFileIdentifiableStore performing R/W operations on the same | ||
| # directory in order to ensure cache validity. The directory is locked as soon as it exists. | ||
| self._dir_lock = DirectoryLock(self.directory_path) | ||
| if os.path.exists(self.directory_path): | ||
| self._acquire_dir_lock() |
There was a problem hiding this comment.
Previously, if the directory did not exist, the store raised a FileNotFoundError during init (I think). This would now raise a RuntimeError because it can't acquire the lock, right? That'd be a breaking change, can you double check please?
There was a problem hiding this comment.
I am pretty sure, that no FileNotFoundError was risen in this case (that's why I implemented it this way). The changes show no removed lines from __init__(). The user was always required to call check_directory() explicitly. This method would raise the error if the parameter create is not set to true.
I do not encourage this design but to not introduce breaking changes, we have to stick with it for now.
| def _lazy_import_fcntl(self): | ||
| try: | ||
| import fcntl as _fcntl | ||
| except ImportError: | ||
| _fcntl = None # Windows: directory locking is unavailable | ||
| return _fcntl |
There was a problem hiding this comment.
I remember we did this to deal with typing issues, but I don't like how it would re-import on every acquire().
Would something like this on the module level?
try:
import fcntl as _fcntl
except ImportError:
_fcntl = None # type: ignore # Windows does not have directory locking| except OSError: | ||
| # dir_lock already taken by other process |
There was a problem hiding this comment.
Isn't OSError a bit generic? According to my AI, contention should raise BlockingIOError specifically.
Catching this would guard against a filesystem without flock support, producing a misleading "already in use by another instance" that sends the user hunting for a nonexistent second process.
Can you verify please?
| with self._write_lock: | ||
| with self._dir_lock.ensure_locked(): | ||
| if os.path.exists( | ||
| "{}/{}.json".format(self.directory_path, self._transform_id(x.id)) | ||
| ): | ||
| raise KeyError( | ||
| "Identifiable with id {} already exists in local file database".format( | ||
| x.id | ||
| ) | ||
| ) | ||
| self._write_atomic(x) | ||
| with self._object_cache_lock: | ||
| self._object_cache[x.id] = x |
There was a problem hiding this comment.
I think this could lead to the object_cache diverging from the actual file's content. Shouldn't the object cache be updated within the write lock?
| with self._write_lock: | ||
| with self._dir_lock.ensure_locked(): | ||
| try: | ||
| os.remove( | ||
| "{}/{}.json".format(self.directory_path, self._transform_id(x.id)) | ||
| ) | ||
| except FileNotFoundError as e: | ||
| raise KeyError( | ||
| "No AAS object with id {} exists in local file database".format(x.id) | ||
| ) from e | ||
| with self._object_cache_lock: | ||
| self._object_cache.pop(x.id, None) |
There was a problem hiding this comment.
My comment at add() is a bit easier to understand here.
Let's say the object gets deleted by thread A.
It finishes deleting the file, gives up the _write_lock.
Then while it's between line 342 and 343, thread B reads the object from the cache, which is now effectivly a ghost.
That's why I think the object_cache update should happen inside the _write_lock.
2 participants
Previously the
LocalFileIdentifiableStorehad no access control for concurrent access through the same or multiple instances on the same directory.This caused serious problems especially if this store is used as storage backend for a multi-worker WSGI server.
Issues with multiple instances
Multiple instances of the
LocalFileIdentifiableStorewere allowed to operate on the same directory. The main issue lies in theself._object_cacheof each instance which does not get updated or invalidated if the other instance is modifying the underlying files through a write operation. In order for the caches to be coherent with the persistent content a inter-process cache synchronization would be necessary. This would introduce to much overhead so we decided to lock the access to a directory to a single instance ofLocalFileIdentifiableStore. This also helps to simplify resolving the following issues.Issues with concurrent calls to a single instance
Concurrent calls to writing methods
add()andcommit()currently race in the_write_atomic()method on theos.replace(...)call. Whenever two threads invoke the method with an identifiable that has the same ID but different content both calls succeed. However, the persisted content depends on which thread gets scheduled to execute theos.replace(...)last.This occurs multiple times across the writing functions. For example in the
add()function the time of check (TOC) for existence may be different that the time of use (TOU) where the new file is created. A concurrently executedadd()might be scheduled such that at TOC both threads find no file for the ID so they both create a new file at TOU. Here again the last scheduled thread silently overwrites the content from the first.Changes
In order to fix the multi-instance issue, these changes introduce a directory lock mechanism, that only allows one instance of the class to hold a lock on a
.lockfile in the directory. A context manager is used to secure operations on the directory and prevent concurrent release of the lock.Caveat: For now this only work on POSIX machines, as it relies on
flockfunctionality which is not provided by Windows.To fix concurrent writes from the same store instance, all write accesses to a file need to be mutual exclusive.
In this implementation I chose to use a global
_writing_lockto lock write operations to all files of the store, as locking individual files has no performance benefit, due to serialization of multiple threads through Pythons GIL.This implementation prevents the following concurrency issues:
add(): Instead of silent overwriting, first call will second fails withKeyError.commit()+discard(): Instead ofcommit()creating file afterdiscard()deleted it, the file is now certainly discarded. Depending on the scheduled ordercommit()may raise aKeyError.Fixes #554