Merge pull request #303 from ZLLentz/fix_save_corrupt

FIX: json backend atomic writes

.github/PULL_REQUEST_TEMPLATE.md

@@ -24,6 +24,6 @@
 - [ ] Code contains descriptive docstrings, including context and API
 - [ ] New/changed functions and methods are covered in the test suite where possible
 - [ ] Test suite passes locally
-- [ ] Test suite passes on continuous integration (Travis CI)
+- [ ] Test suite passes on GitHub Actions
 - [ ] Ran ``docs/pre-release-notes.sh`` and created a pre-release documentation page
 - [ ] Pre-release docs include context, functional descriptions, and contributors as appropriate

README.md

@@ -12,15 +12,6 @@
   <a href="https://pcdshub.github.io/happi/">Documentation</a>
 </p>
 
-<div align="center">
-  <!-- Build Status -->
-  <a href="https://travis-ci.org/pcdshub/happi">
-    <img
-src="https://img.shields.io/travis/pcdshub/happi/master.svg?style=flat-square"
-      alt="Build Status" />
-  </a>
-</div>
-
 ## Motivation
 LCLS endstations deal with dynamic sets of instrumentation. Information like
 ports, triggers and aliases are all important for operation, but hard to manage

docs/source/upcoming_release_notes/301-fix_save_corrupt.rst

@@ -0,0 +1,24 @@
+301 fix_save_corrupt
+####################
+
+API Changes
+-----------
+- N/A
+
+Features
+--------
+- N/A
+
+Bugfixes
+--------
+- Fix an issue where an ill-timed interrupt of the json backend's
+  ``store`` operation could truncate the data file. This also removes
+  the implicit/optional dependency on ``fcntl``.
+
+Maintenance
+-----------
+- Remove some lingering references to Travis CI
+
+Contributors
+------------
+- zllentz

happi/backends/json_db.py

@@ -2,11 +2,15 @@
 Backend implemenation using the ``simplejson`` package.
 """
 import contextlib
+import getpass
 import logging
 import math
 import os
 import os.path
 import re
+import shutil
+import time
+import uuid
 from typing import Any, Callable, Optional, Union
 
 import simplejson as json
@@ -17,14 +21,6 @@
 
 logger = logging.getLogger(__name__)
 
-
-try:
-    import fcntl
-except ImportError:
-    logger.debug("Unable to import 'fcntl'. Will be unable to lock files")
-    fcntl = None
-
-
 # A sentinel for keys that are missing for comparisons below.
 _MISSING = object()
 
@@ -128,29 +124,45 @@ def store(self, db: dict[str, ItemMeta]) -> None:
         """
         Stash the database in the JSON file.
 
+        This is a two-step process:
+
+        1. Write the database out to a temporary file
+        2. Move the temporary file over the previous database.
+
+        Step 2 is an atomic operation, ensuring that the database
+        does not get corrupted by an interrupted json.dump.
+
         Parameters
         ----------
         db : dict
             Dictionary to store in JSON.
-
-        Raises
-        ------
-        BlockingIOError
-            If the file is already being used by another happi operation.
         """
+        temp_path = self._temp_path()
 
-        with open(self.path, 'w+') as f:
-            # Create lock in filesystem
-            if fcntl is not None:
-                fcntl.flock(f, fcntl.LOCK_EX | fcntl.LOCK_NB)
-            # Dump to file
-            try:
-                json.dump(db, f, sort_keys=True, indent=4)
+        with open(temp_path, 'w') as fd:
+            json.dump(db, fd, sort_keys=True, indent=4)
+
+        if os.path.exists(self.path):
+            shutil.copymode(self.path, temp_path)
+        shutil.move(temp_path, self.path)
 
-            finally:
-                if fcntl is not None:
-                    # Release lock in filesystem
-                    fcntl.flock(f, fcntl.LOCK_UN)
+    def _temp_path(self) -> str:
+        """
+        Return a temporary path to write the json file to during "store".
+
+        Includes a username and a timestamp for sorting
+        (in the cases where the temp file isn't cleaned up)
+        and a hash for uniqueness
+        (in the cases where multiple temp files are written at once).
+        """
+        directory = os.path.dirname(self.path)
+        filename = (
+            f".{getpass.getuser()}"
+            f"_{int(time.time())}"
+            f"_{str(uuid.uuid4())[:8]}"
+            f"_{os.path.basename(self.path)}"
+        )
+        return os.path.join(directory, filename)
 
     def _iterative_compare(self, comparison: Callable) -> ItemMetaGen:
         """

happi/tests/test_backends.py

@@ -1,4 +1,3 @@
-import fcntl
 import os
 import os.path
 import tempfile
@@ -178,15 +177,6 @@ def test_json_save(mockjson, item_info: dict[str, Any], valve_info):
     assert valve_info in mockjson.all_items
 
 
-def test_json_locking(mockjson):
-    # Place lock on file
-    handle = open(mockjson.path, 'w')
-    fcntl.flock(handle, fcntl.LOCK_EX | fcntl.LOCK_NB)
-    # Attempt to save
-    with pytest.raises(IOError):
-        mockjson.store({"_ID": "ID"})
-
-
 def test_json_initialize():
     jb = JSONBackend("testing.json", initialize=True)
     # Check that the file was made
@@ -200,6 +190,19 @@ def test_json_initialize():
     os.remove("testing.json")
 
 
+def test_json_tempfile_location():
+    jb = JSONBackend("testing.json", initialize=False)
+    assert os.path.dirname(jb.path) == os.path.dirname(jb._temp_path())
+
+
+def test_json_tempfile_uniqueness():
+    jb = JSONBackend("testing.json", initialize=False)
+    tempfiles = []
+    for _ in range(100):
+        tempfiles.append(jb._temp_path())
+    assert len(set(tempfiles)) == len(tempfiles)
+
+
 @requires_questionnaire
 def test_qs_find(mockqsbackend):
     assert len(list(mockqsbackend.find(dict(beamline='TST')))) == 14