From a9cf9d55c8be986bcecc07a0ad3e24f71854d89e Mon Sep 17 00:00:00 2001
From: Laurent Monin <github@norz.org>
Date: Tue, 19 Mar 2019 17:17:48 +0100
Subject: [PATCH] File._preserve_times(): fix broken logic

- access times are likely to differ, a race condition is possible
- stat() has be done after the change to be able to compare
- comments added
- force sync after modifying test file
- make code easier to understand

See https://github.com/metabrainz/picard/pull/1132#issuecomment-474113665
---
 picard/file.py    |  5 ++---
 test/test_file.py | 27 +++++++++++++++++++++------
 2 files changed, 23 insertions(+), 9 deletions(-)

diff --git a/picard/file.py b/picard/file.py
index 3afd17e9b..e5e365d09 100644
--- a/picard/file.py
+++ b/picard/file.py
@@ -221,18 +221,17 @@ class File(QtCore.QObject, Item):
             # The best way to preserve exact times is to use the st_atime_ns and st_mtime_ns
             # fields from the os.stat() result object with the ns parameter to utime.
             st = os.stat(filename)
-            times_ns = (st.st_atime_ns, st.st_mtime_ns)
         except OSError as why:
             errmsg = "Couldn't read timestamps from %r: %s" % (filename, why)
             raise self.PreserveTimesStatError(errmsg) from None
             # if we can't read original times, don't call func and let caller handle this
         func()
         try:
-            os.utime(filename, ns=times_ns)
+            os.utime(filename, ns=(st.st_atime_ns, st.st_mtime_ns))
         except OSError as why:
             errmsg = "Couldn't preserve timestamps for %r: %s" % (filename, why)
             raise self.PreserveTimesUtimeError(errmsg) from None
-        return times_ns
+        return (st.st_atime_ns, st.st_mtime_ns)
 
     def _save_and_rename(self, old_filename, metadata):
         """Save the metadata."""
diff --git a/test/test_file.py b/test/test_file.py
index 4d43f9c1c..9dbf1d327 100644
--- a/test/test_file.py
+++ b/test/test_file.py
@@ -47,11 +47,15 @@ class TestPreserveTimes(PicardTestCase):
         # create a dummy file
         with open(self.file.filename, 'w') as f:
             f.write('xxx')
+            f.flush()
+            os.fsync(f.fileno())
 
     def _modify_testfile(self):
         # dummy file modification, append data to it
         with open(self.file.filename, 'a') as f:
             f.write('yyy')
+            f.flush()
+            os.fsync(f.fileno())
 
     def _read_testfile(self):
         with open(self.file.filename, 'r') as f:
@@ -60,15 +64,26 @@ class TestPreserveTimes(PicardTestCase):
     def test_preserve_times(self):
         self._create_testfile()
 
-        st = os.stat(self.file.filename)
-        expect = (st.st_mtime_ns, st.st_atime_ns)
-
         # test if times are preserved
-        result = self.file._preserve_times(self.file.filename,
-                                           self._modify_testfile)
-        self.assertEqual(result, expect)
+        (before_atime_ns, before_mtime_ns) = self.file._preserve_times(self.file.filename, self._modify_testfile)
+
+        # HERE an external access to the file is possible, modifying its access time
+
+        # read times again and compare with original
+        st = os.stat(self.file.filename)
+        (after_atimes_ns, after_mtimes_ns) = (st.st_atime_ns, st.st_mtime_ns)
+
+        # modification times should be equal
+        self.assertEqual(before_mtime_ns, after_mtimes_ns)
+
+        # access times may not be equal
+        # time difference should be positive and reasonably low (if no access in between, it should be 0)
+        delta = after_atimes_ns - before_atime_ns
+        tolerance = 10**7  #  0.01 seconds
+        self.assertTrue(0 <= delta < tolerance)
 
         # ensure written data can be read back
+        # keep it at the end, we don't want to access file before time checks
         self.assertEqual(self._read_testfile(), 'xxxyyy')
 
     def test_preserve_times_nofile(self):