GitLab

Andreas Klebinger pushed to branch wip/andreask/hadrian_race at Glasgow Haskell Compiler / GHC

Commits:

20a0c08f

by Andreas Klebinger at 2026-02-20T13:51:25+00:00

Apply 5 suggestion(s) to 2 file(s)

Co-authored-by: sheaf <sam.derbyshire@gmail.com>

2 changed files:

libraries/ghc-boot/GHC/Unit/Database.hs
utils/ghc-pkg/Main.hs

Changes:

libraries/ghc-boot/GHC/Unit/Database.hs

@@ -317,17 +317,29 @@ data DbInstUnitId
  -- | Represents a lock of a package db.
  newtype PackageDbLock = PackageDbLock Handle
 --- | Run the action under a lock, then return the result.
 --- If the mode is R/W the *caller* needs to either free the lock or pass it
 --- on to code that will.
 +-- | Take a lock on the package database and then run the action.
  --
 --- If an exception is raised the lock is released.
 +--   - In read-only mode, this takes and releases a shared lock.
 +--   - In read-write mode, this takes an exclusive lock, and the caller
 +--     must arrange for this lock to be released:
 +--
 +--        - either the inner action releases it, or
 +--        - the inner action returns the lock and the caller
 +--          of 'withLockedPackageDb' is responsible for releasing it.
 +--
 +-- If an exception escapes the inner action, the lock is released.
 +--
 +-- See Note [ghc-pkg database locking] in ghc-pkg/Main.hs
  withLockedPackageDb :: DbOpenMode m t -> FilePath -> (PackageDbLock -> IO a) -> IO a
 -withLockedPackageDb mode file act = do
 -   lock <- lockPackageDbWith (lock_mode mode) file
 -   r <- act lock `onException` unlockPackageDb lock
 -   when (isDbOpenReadMode mode ) $ unlockPackageDb lock
 -   pure r
 +withLockedPackageDb mode file =
 +  bracket_for_mode
 +    (lockPackageDbWith (lock_mode mode) file)
 +    unlockPackageDb
 +  where
 +    bracket_for_mode =
 +      case mode of
 +        DbOpenReadOnly  -> bracket
 +        DbOpenReadWrite -> bracketOnError
    where
     lock_mode :: DbOpenMode m t -> LockMode
     lock_mode DbOpenReadOnly = SharedLock
@@ -529,8 +541,8 @@ headerMagic = BS.Char8.pack "\0ghcpkg\0"
  -- | Feed a 'Get' decoder with data chunks from a file.
  --
 --- The file is already locked when we call this. We only need to pass it on
 --- if we are in R/W mode.
 +-- Requires a lock (either shared or exclusive) on the package database,
 +-- which it returns unchanged.
  decodeFromFile :: FilePath -> DbOpenMode mode PackageDbLock -> Get pkgs ->
                    IO (pkgs, DbOpenMode mode PackageDbLock)
  decodeFromFile file mode decoder = case mode of

utils/ghc-pkg/Main.hs

@@ -875,31 +875,30 @@ lookForPackageDBIn dir = do
  {- Note [ghc-pkg database locking]
  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 -It's important for ghc-pkg, GHC and hadrian to always see a consistent state for
 -package databases. To ensure this we generally tacke a lock both for reading and
 -writing to the package database.
+-
 -The general idea is we use `withLockedPackageDb` to lock an already existing
 -database in both modes. Which behaves differently for reads and modify.
+-
 -In read mode we take a shared lock before we start reading, run the argument to
 -withLockedPackageDb and simply unlock the DB after. We need to lock it to avoid
 -concurrent invocations from deleting files from the db before we managed to read
 -them. This occasionally happened in #22870. Not that historically we only used
 -to lock the package db during reads on windows, for reasons that seem to have been
 -bogus given the existence of #22870. And it was partially discussed in #16773.
+-
 -When modifying a package database we also use withLockedPackageDb but we take
 -an *exclusive* lock and most importantly we don't unlock the DB after the action
 -has run unless an exception occurs.
 -Instead the action we are must ensure the lock is either freed, or returned
 -as part of the result to be freed later by the caller of withLockedPackageDB.
 -Typically by storing it inside either a `DbOpenMode` or `PackageDB`.
+-
 -While this setup is a bit cumbersome it seemed to be the easiest way to ensure
 +It's important for ghc-pkg, GHC and Hadrian to always see a consistent state for
 +package databases. To ensure this, we always take a lock before reading or writing
 +to a package database.
++
 +The general idea is we use 'withLockedPackageDb' to lock an already existing
 +database in both modes:
++
 +  - In read-only mode, we take a *shared* lock before reading and unlock afterwards.
 +    Locking is needed to avoid a concurrent invocation of ghc-pkg deleting files from
 +    the db before we manage to read them. This occasionally happened in #22870.
 +    (Note that, historically, we used to only lock the package db during reads on
 +     Windows, but the justification seemed insufficient given #22870. See also #16773.)
++
 +  - In read-write mode, we take an **exclusive** lock on the package database.
 +    However, we don't automatically release the lock after the inner action completes
 +    (unless an exception occurs). Instead, either the inner action itself releases the
 +    lock, or it returns it, in which case it is the caller of 'withLockedPackageDb'
 +    that is responsible for releasing the lock. Typically, the exclusive lock is stored
 +    in 'PackageDB', with the type informing us that we have a lock we are responsible for
 +    releasing. 'updateDBCache' eventually releases it.
++
 +While this setup is a bit cumbersome, it seemed to be the easiest way to ensure
  that locks are consistently held over reads/modifications of a database without
  engaging in a larger refactor of the ghc-pkg code.
+-
  -}
  readParseDatabase :: forall mode t. Verbosity
                    -> Maybe (FilePath,Bool)
@@ -923,7 +922,7 @@ readParseDatabase verbosity mb_user_conf mode use_cache path
           -- Take a lock to use while we read the DB
           Right fs -> withLockedPackageDb mode cache $ \lock -> do
            if not use_cache
 -            then ignore_cache (lock) (const $ return ())
 +            then ignore_cache lock (const $ return ())
              else do
                    e_tcache <- tryIO $ getModificationTime cache
                    case e_tcache of