Add support for concurrent package db access and updates
ClosedPublic

Authored by arybczak on Feb 5 2017, 9:55 PM.

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
There are a very large number of changes, so older changes are hidden. Show Older Changes
arybczak updated this revision to Diff 10987.Feb 6 2017, 1:24 PM
  • Update messages and fix tests
bgamari added inline comments.Feb 6 2017, 6:59 PM
libraries/base/GHC/IO/Handle/Lock.hsc
110

You will need to import GHC.Real for this (with proper CPP guards to avoid unused import warnings). I don't believe this will introduce an import loop.

libraries/ghc-boot/GHC/PackageDb.hs
316

It really feels like this should be two functions: One which just reads the database and another which reads and gives you the handle.

333–336

Let's document what atomicity guarantees this gives you in both Left and Right cases.

bgamari added inline comments.Feb 6 2017, 7:05 PM
utils/ghc-pkg/Main.hs
793

The interface and behavior of this function is a bit hard to wrap your head around. We at very least need some documentation here.

797–798

What does this Bool mean? Perhaps a dedicated type would help here?

867–871

What does this Bool mean?

duncan edited edge metadata.Feb 6 2017, 7:08 PM

Not yet reviewed the ghc-pkg changes, but the first parts look good.

libraries/base/GHC/IO/Handle/Lock.hsc
5

Is everyone happy with lambda case and multi-way if in ghc/lib code?

This is really a Q to the other reviewers.

58

I wonder if we should also note that this requires the threaded runtime system to avoid blocking all threads, and is moderately expensive on the threaded rts as it will use a whole OS thread (for the safe FFI call).

73

Was there not already an impl of this somewhere?

129

Might as well include a comment here with the link to the Win32 API docs so it's easier for people later to see what things mean
https://msdn.microsoft.com/en-us/library/windows/desktop/aa365203(v=vs.85).aspx

dfeuer added a subscriber: dfeuer.Feb 6 2017, 11:50 PM
dfeuer added inline comments.
libraries/base/GHC/IO/Handle/Lock.hsc
5

I don't see why not. This is "internal" GHC code; all well-behaved extensions with two versions worth of support are presumably fair game.

18

There's an awful lot of if-then clutter here. Can you add some comments to clarify which endif goes with which?

71

If there's no other such function around, I think it would be nice to have a total version of this as the basic one (ignore my lousy naming)

data Problem = NotFileDescriptor | NotFileHandle

handleToFD :: Handle -> IO (Either Problem FD)
handleToFD h = case h of
  FileHandle _ mv -> do
    Handle__{haDevice = dev} <- readMVar mv
    maybe (pure NotFileDescriptor) pure (cast dev)
  DuplexHandle{} -> pure NotFileHandle
84

This looks like it could use some refactoring. The cmode conversion could be a top-level function defined right by LockMode. I think you could write a sort of catchErrno to abstract out errno handling. Then once that's done, inline lockImpl into its call sites. Feel free to disagree, but I don't see hLock and hTryLock as having enough in common to justify a unified implementation.

bgamari added inline comments.Feb 7 2017, 12:10 AM
libraries/base/GHC/IO/Handle/Lock.hsc
58

Ahh, yes, indeed we should.

73

I could be wrong but I believe the only implementation is in unix. GHC.IO.Handle.FD has a fdToHandle implementation, so perhaps this should end up there. The only problem is that the treatment of DuplexHandle is a bit application-specific.

arybczak updated this revision to Diff 10998.Feb 7 2017, 12:26 AM
  • Fix compilation on Windows
Phyx added a subscriber: Phyx.Feb 7 2017, 4:51 AM
Phyx added inline comments.
libraries/base/GHC/IO/Handle/Lock.hsc
73

Win32 also has a conversion function for this hANDLEToHandle. It maps from the pseudo FD handles of the POSIX part of the API. But it's not available for use in stage1 yet.

129

you can drop the (v=vs.85) part of the link to make it a bit more stable as the visual studio version is irrelevant in this case.

duncan added a comment.Feb 7 2017, 1:04 PM

So I think in the ghc-boot code it would be nicer to factor things like this:

simple read-only path, with api like now:

readPackageDbForGhcPkg :: Binary pkgs => FilePath -> IO pkgs

and then for the write code path, something like:

newtype PackageDbHandle = PackageDbHandle Handle

openPackageDb :: FilePath -> IO PackageDbHandle
getPackageDbForGhcPkg :: Binary pkgs => PackageDbHandle -> IO pkgs
putPackageDb :: (Binary pkgs, RepBlahBlah...) => PackageDbHandle -> [InstalledPackageInfo a b c d e f g] -> pkgs -> IO ()
closePackageDb :: PackageDbHandle -> IO ()

where the openPackageDb would open in read/write mode and lock the file for writing. The getPackageDbForGhcPkg would do the read and reset the file offset to 0. The putPackageDb would set the file size to 0 and write the new content. The closePackageDb would just close the handle (which unlocks the file).

In the case that openPackageDb cannot lock (due to locking API not being available yet) then it'd just open the file but not lock.

This may also mean that in the ghc-pkg code we would need to more work to clearly distinguish the reading and writing cases, but that's probably a good thing.

libraries/ghc-boot/GHC/PackageDb.hs
333–336

I don't yet understand why we would still need a code path where we write directly without first opening & locking.

408

I think we could factor this slightly better:

decodeFromHandle :: Handle -> Get a -> IO a
decodeFromHandle hnd decoder =
    feed hnd (runGetIncremental decoder)

that just does the feed bit, and then have two variants, the old read-only decodeFromFile

decodeFromFile :: FilePath -> Get a -> IO a
decodeFromFile file decoder =
    withBinaryFile file ReadMode $ \hnd ->
      decodeFromHandle hnd decoder

Then for the other code path, with the read-write & locking, I feel a bit uncomfortable about conflating the two modes in one function like ​readPackageDbForGhcPkg. I think it'd be better if we can clearly separate the simple read-only path from the complex path of: open & lock, read while locked, write and unlock.

duncan added a comment.Feb 7 2017, 1:16 PM

I wonder if we can get ghc-pkg to be more disciplined in the types for opening for read vs read/write. I'll see if I can come up with a GADT trick...

utils/ghc-pkg/Main.hs
1224

How can this case happen? Surely if we're writing then we must have previously opened for read/write. This is related to why we need the simple "just write to this file" case in writePackageDb above. I don't see why we would need to do this. Can't we simply guarantee that if we're going to be modifying then we'll open for read/write mode with locking.

Phyx removed a subscriber: Phyx.Feb 7 2017, 1:27 PM

Here's a sketch for how to get the ghc-pkg Main code a bit clearer on the read vs read/write cases

https://gist.github.com/dcoutts/fc343ef68c9177423824bdb187a67415

We'd adapt this idea so that we return the Handle rather than the FilePath in the read/write case, and thereby avoid changing the PackageDB type with the maybe handle.

bgamari requested changes to this revision.Feb 8 2017, 12:38 PM

@duncan, indeed that looks like it would be an improvement.

Bumping out of review queue for now.

This revision now requires changes to proceed.Feb 8 2017, 12:38 PM
arybczak marked an inline comment as done.Feb 8 2017, 1:41 PM

We'd adapt this idea so that we return the Handle rather than the FilePath in the read/write case, and thereby avoid changing the PackageDB type with the maybe handle.

I'm not sure this bit is going to be an improvement - I tried to do it like this in the beginning and it was messy, because you need to pass/return Handle in the inner functions in addition to PackageDB. You also don't get a PackageDB directly and need to do a search for the actual package db to be modified each time you return from getPkgDatabases.

Possibly having PackageDB with a tag whether it's for modification or not could work.

There is also the issue of compatibility between base >= 4.10.0.0 and below - currently you just get PackageDB for modification and if it's base >= 4.10.0.0, it will have a Handle inside and otherwise it won't (well, it is possible to not have a Handle in the first case at the moment since cabal just shows a warning if that happens, but it could be made an error) - if you want to statically guarantee we have a Handle when we write, that requires sticking some additional ifdefs around.

We'd adapt this idea so that we return the Handle rather than the FilePath in the read/write case, and thereby avoid changing the PackageDB type with the maybe handle.

I'm not sure this bit is going to be an improvement - I tried to do it like this in the beginning and it was messy, because you need to pass/return Handle in the inner functions in addition to PackageDB. You also don't get a PackageDB directly and need to do a search for the actual package db to be modified each time you return from getPkgDatabases.

Possibly having PackageDB with a tag whether it's for modification or not could work.

This sounds plausible.

I do think the status quo needs to be cleaned up if we are going to add this complexity. The current interface is just too easy to misuse and does not feel right.

arybczak added inline comments.Feb 10 2017, 2:17 AM
libraries/base/GHC/IO/Handle/Lock.hsc
84

I disagree. Splitting it into a couple of smaller functions that are necessarily ifdef'ed would hurt readability (imho).

arybczak updated this revision to Diff 11087.Feb 10 2017, 3:21 AM
arybczak edited edge metadata.
  • Use GADTs to clearly distinguish between reading/writing case
arybczak marked 8 inline comments as done.Feb 10 2017, 3:30 AM

Here's take two, using GADTs to clearly distinguish what we do in read only and read write case. I also changed locking strategy - using separate lock file allows us to easily retain update using atomic rename (which is a sane thing to do, unlike truncating in place I feel).

I didn't split reading function in ghc-boot into two, because:

  1. It should be clear now because of a bit of type trickery.
  2. Things just fall into place.
  3. If it's split, it's hard to come up with sensible names for these functions and locally defined functions would need to be put on the top level, which is a bit messy.

Let's discuss.

arybczak updated this revision to Diff 11088.Feb 10 2017, 4:33 AM
  • Remove unused veriable
dfeuer requested changes to this revision.Feb 14 2017, 8:36 PM
dfeuer added inline comments.
libraries/ghc-boot/GHC/PackageDb.hs
224

Can you add a Haddock comment for lockPackageDbWith?

228

Can you explain this a bit better? If we don't have write permission when we first lock the file, why will we be able to lock it in write mode in lockFileOpenIn?

259

Can you document what t means in DbOpenMode?

299

You're wasting the do. I think you should probably just use it:

(pkgs, DbOpenReadOnly) <- decodeFromFile file DbOpenReadOnly getDbForGhc
pure pkgs

but the alternative is to get rid of it.

utils/ghc-pkg/Main.hs
688

What's the significance of the last element of db_flags?

692

What is this approximation possibly_modified for? What exactly does it mean? A couple more comments wouldn't hurt.

695

Do we have a clone of Control.Monad.Trans.State.Strict lying around that we could use here?

868

It might be worth commenting that this mapM (and the other one) locks the package precisely when using ReadWriteMode, and otherwise leaves the package unlocked. Can you explain why the latter is safe?

This revision now requires changes to proceed.Feb 14 2017, 8:36 PM
arybczak added inline comments.Feb 15 2017, 6:36 AM
libraries/ghc-boot/GHC/PackageDb.hs
299

It's intentional, matching in do block doesn't warn you about incomplete pattern, matching with case does.

utils/ghc-pkg/Main.hs
688

I don't know.

868

It follows from the definition of OpenDbMode, which is essentially a tagged Maybe, doesn't it?

duncan requested changes to this revision.Feb 15 2017, 7:24 AM

So this is a whole lot better. It really clarifies when we're holding locks. I like it.

Just a few suggestions. The main one is to try and make the horrible "look for which package db contains this pkg" feature a bit more comprehensible. We can't do anything about the fact that it interacts badly with locking, we can just try our best to separate that code out and explain clearly what we're having to do.

libraries/base/GHC/IO/Handle/Lock.hsc
5

Ok, fine.

libraries/ghc-boot/GHC/PackageDb.hs
228

Let me try and add what I think the code comment here ought to be:

We are trying to open the lock file and then lock it. Thus the lock file needs to either exist or we need to be able to create it. Ideally we would not assume that the lock file always exists in advance. When we are dealing with a package Db where we have write access then if the lock file does not exist then we can create it by opening the file in read/write mode. On the other hand if we are dealing with a package Db where we do not have write access (e.g. a global Db) then we can only open in read mode, and the lock file had better exist already or we're in trouble. So for global read-only Dbs on platforms where we must lock the Db for reading then we will require that the installer/packaging has included the lock file.

Thus the logic here is to first try opening in read-only mode (to handle global read-only Dbs) and but if the file does not exist then try opening in read/write mode so as to create the lock file. If either succeed then lock the file. IO exceptions (other than the first open attempt failing due to the file not existing) simply propagate.

236

Could this use of mask be a bracketOnError instead?

bracketOnError
  (openBinaryFile lock io_mode)
  hClose
  (\hnd -> do hLock hnd mode; return (PackageDbLock hnd))
259

It's like the a in Maybe a. Yes this should be documented. e.g.

-- | 'DbOpenMode' holds a value of type @t@ but only in 'DbReadWrite' mode.  So it is like
-- 'Maybe' but with a type argument for the mode to enforce that the mode is used consistently.
299

or just

fst <$> decodeFromFile file DbOpenReadOnly getDbForGhc
333–336

Ok, the previous comments here are now out of date. But I have another suggestion which is that we make the function take the PackageDbLock as an arg (though it would not do anything with it), just to clarify in the API here that we're only supposed to write when we have the lock.

utils/ghc-pkg/Main.hs
688

The db stack is represented with the top most db at the end of the list, e.g.

[Global, User, Specific "./package.db.d"]
690

Sorry I don't think it's sufficiently clear here what the lambda is that we can use the short lambda case style. Use the full \a_helpful_name -> case a_helpful_name of.

692

Sigh, the "look for which package db contains this pkg" feature really adds complexity doesn't it. Especially when working out which dbs to lock. I wonder if just for our sanity we can at least pull out the code to support the ContainsPkg case into a separate helper function. It doesn't reduce complexity but it might help reading the code and modifying in future.

726

I initially thought this was wrong, so we should comment as to why it's right.

I thought that we would still need to hold a read lock, that we can't downgrade from a write lock to no lock. But actually it's fine since readParseDatabase will open, read and close the db cache file, so we have already finished reading, so we if it turns out we only needed to read then we've done it and can indeed release the write lock.

734

Ok so I think I see what's going on now, but yes I think this could benefit from being pulled out and commented separately, since it is complicated.

Here's the summary I think:

We go through the final db stack reading each package db, but we need to lock the db that we are going to modify.

  1. In one case we know it's the top most and so we know its location in advance, so when we get to that one we can just open it in read/write mode.
  2. In the complex case we don't know until we've looked inside the db if we're going to want to modify this db (since we modify the one where we find the package we're interested in). We open each one in read/write mode initially and downgrade that to a read-only lock if it turns out we will not need to modify it.

I now wonder if we could make this easier to understand if we separated out the two selector cases and do them separately, as we already do the read only mode case separately. That is we do three cases with a helper functions like:

-- returns the (db_stack, db_to_operate_on):
getPkgDatabasesHelper :: ... -> IO (PackageDBStack, GhcPkg.DbOpenMode mode (PackageDB mode))
getPkgDatabasesHelper final_stack GhcPkg.DbOpenReadOnly = do
    -- simple, we just read all the dbs in the stack in read only mode
    ...

getPkgDatabasesHelper GhcPkg.DbOpenReadWrite TopOne = do
    -- a bit more complicated, we read all the dbs in the stack in read only mode
    -- except for the top most db which we open in read/write mode
    ...

getPkgDatabasesHelper GhcPkg.DbOpenReadWrite (ContainsPkg pkgarg) = do
    -- this is the tricky case, we don't know which db will contain the package we're looking for
    -- but whichever one it is we want to open that db in read/write mode
    ...
DemiMarie added inline comments.
libraries/base/GHC/IO/Handle/Lock.hsc
153

I don't think we should silently do nothing here, since this could cause silent corruption.

Better to make this work on Solaris, and throw an exception elsewhere.

duncan added inline comments.Feb 15 2017, 5:19 PM
libraries/base/GHC/IO/Handle/Lock.hsc
153

Note that this "no locking at all" situation is exactly what we have now.

I'd also be nervous of adding posix fcntl locking support since unfortunately the semantics is bonkers. It's not unlikely that this support for file locking will be exposed in a public API at some stage (e.g System.IO), and so having sane consistent semantics is important. Linux supports a non-bonkers non-posix fcntl-based locking API, but that doesn't help solaris.

Does solaris support any file locking other than the broken posix fcntl API?

And yes, if it's exposed publicly then we'll have to throw an exception for Handles or platforms that cannot support locking, and the use in ghc-pkg will have to be adjusted to catch this exception and proceed without locking.

arybczak updated this revision to Diff 11272.Feb 23 2017, 4:35 PM
arybczak edited edge metadata.
arybczak marked 18 inline comments as done.
  • Fixes
arybczak added a comment.EditedFeb 23 2017, 4:36 PM

I moved handleToFd to GHC.IO.Handle.FD, which required moving isEOF to GHC.IO.Handle to avoid cyclic imports.

libraries/base/GHC/IO/Handle/Lock.hsc
153

Does solaris support any file locking other than the broken posix fcntl API?

None that I know of.

I changed the implementation to throw an exception instead, that seems to be more sensible.

utils/ghc-pkg/Main.hs
868

Never mind, I misunderstood your comment.

bgamari accepted this revision.Feb 26 2017, 3:34 PM
bgamari updated the Trac tickets for this revision.

Looks good to me.

libraries/ghc-boot/GHC/PackageDb.hs
214

A comment clarifying that we were not able to lock prior to base-4.10.0 would be helpful here.

This revision was automatically updated to reflect the committed changes.