Fix ghc-pkg reports cache out date (#10205)
authorThomas Miedema <thomasmiedema@gmail.com>
Tue, 16 Jun 2015 21:38:38 +0000 (16:38 -0500)
committerAustin Seipp <austin@well-typed.com>
Tue, 16 Jun 2015 21:40:09 +0000 (16:40 -0500)
See Note [writeAtomic leaky abstraction].

GHC on Linux already received a patch for this bug in
e0801a0fb342eea9a312906eab72874d631271cf. On Windows several cabal tests
were hitting the bug, causing validate failures, but we never noticed
because of all the other tests that were failing on Windows. And it
didn't start happening till `getModificationTime` received sub-second
resolution support on Windows in
5cf76186d373842bf64d49cecb09e0a9ddce3203.

Since there are regression tests already, I am not adding another one.
But for good measure, here is a script that shows the bug without
needing to do a full validate run:

  DB=/tmp/package.conf.d.test
  GHC_PKG=ghc-pkg #utils/ghc-pkg/dist/build/tmp/ghc-pkg
  LOCAL_GHC_PKG="${GHC_PKG} --no-user-package-db --global-package-db=${DB}"
  while true; do
    rm -rf ${DB}
    ${LOCAL_GHC_PKG} init "${DB}"
    ${LOCAL_GHC_PKG} list
  done

If you see "WARNING: cache is out of date" after a few seconds, the bug
is not fixed.

Reviewed By: austin

Differential Revision: https://phabricator.haskell.org/D990

GHC Trac Issues: #10205

utils/ghc-pkg/Main.hs

index b7e617e..f3017a2 100644 (file)
@@ -1077,10 +1077,15 @@ updateDBCache verbosity db = do
       if isPermissionError e
       then die (filename ++ ": you don't have permission to modify this file")
       else ioError e
-#ifndef mingw32_HOST_OS
-  status <- getFileStatus filename
-  setFileTimes (location db) (accessTime status) (modificationTime status)
-#endif
+  -- See Note [writeAtomic leaky abstraction]
+  -- Cross-platform "touch". This only works if filename is not empty, and not
+  -- open for writing already.
+  -- TODO. When the Win32 or directory packages have either a touchFile or a
+  -- setModificationTime function, use one of those.
+  withBinaryFile filename ReadWriteMode $ \handle -> do
+      c <- hGetChar handle
+      hSeek handle AbsoluteSeek 0
+      hPutChar handle c
 
 type PackageCacheFormat = GhcPkg.InstalledPackageInfo
                             String     -- installed package id
@@ -2045,3 +2050,38 @@ removeFileSafe fn =
 -- absolute path.
 absolutePath :: FilePath -> IO FilePath
 absolutePath path = return . normalise . (</> path) =<< getCurrentDirectory
+
+
+{- Note [writeAtomic leaky abstraction]
+GhcPkg.writePackageDb calls writeAtomic, which first writes to a temp file,
+and then moves the tempfile to its final destination. This all happens in the
+same directory (package.conf.d).
+Moving a file doesn't change its modification time, but it *does* change the
+modification time of the directory it is placed in. Since we compare the
+modification time of the cache file to that of the directory it is in to
+decide whether the cache is out-of-date, it will be instantly out-of-date
+after creation, if the renaming takes longer than the smallest time difference
+that the getModificationTime can measure.
+
+The solution we opt for is a "touch" of the cache file right after it is
+created. This resets the modification time of the cache file and the directory
+to the current time.
+
+Other possible solutions:
+  * backdate the modification time of the directory to the modification time
+    of the cachefile. This is what we used to do on posix platforms. An
+    observer of the directory would see the modification time of the directory
+    jump back in time. Not nice, although in practice probably not a problem.
+    Also note that a cross-platform implementation of setModificationTime is
+    currently not available.
+  * set the modification time of the cache file to the modification time of
+    the directory (instead of the curent time). This could also work,
+    given that we are the only ones writing to this directory. It would also
+    require a high-precision getModificationTime (lower precision times get
+    rounded down it seems), or the cache would still be out-of-date.
+  * change writeAtomic to create the tempfile outside of the target file's
+    directory.
+  * create the cachefile outside of the package.conf.d directory in the first
+    place. But there are tests and there might be tools that currently rely on
+    the package.conf.d/package.cache format.
+-}