Write cache files atomically #1544
Conversation
dhall/src/Dhall/Import.hs
Outdated
| @@ -554,7 +555,7 @@ writeToSemanticCache :: Dhall.Crypto.SHA256Digest -> Data.ByteString.ByteString | |||
| writeToSemanticCache hash bytes = do | |||
| _ <- Maybe.runMaybeT $ do | |||
| cacheFile <- getCacheFile "dhall" hash | |||
| liftIO (Data.ByteString.writeFile cacheFile bytes) | |||
| liftIO (System.AtomicWrite.Writer.ByteString.atomicWriteFile cacheFile bytes) | |||
There was a problem hiding this comment.
I'm not yet sure whether we ought to use
with some "binary" FileMode instead. I can't actually find any documentation on constructing these FileModes.
There was a problem hiding this comment.
Ah no, FileMode seems to be about file permissions instead of text mode vs binary mode.
There was a problem hiding this comment.
I've asked for clarification regarding newline adjustments in stackbuilders/atomic-write#16.
There was a problem hiding this comment.
@sjakobi: Yeah, I don't believe binary mode is possible using the current atomic-write API
There was a problem hiding this comment.
I'll wait a bit for feedback from the atomic-write maintainers. Not sure whether I should then make a patch to add binary mode writes to atomic-write, or just switch to unliftio (which however doesn't seem to implement atomicity on Windows).
It would be nice if we could use the same package to fix #498 too…
There was a problem hiding this comment.
I have opened fpco/unliftio#50 regarding Windows support and fpco/unliftio#51 regarding support for text mode in unliftio.
There was a problem hiding this comment.
Feel free to switch to unliftio if that ends up being easier
There was a problem hiding this comment.
atomic-write now offers binary mode writes too, which I'm using now.
|
According to stackbuilders/atomic-write#9, we might write over pre-existing read-only files here. I'm not overly worried about this since we only write files in the |
sjakobi
force-pushed
the
sjakobi/1540-atomic-cache
branch
from
6f40348
to
4e9883e
Compare
sjakobi
changed the title
|
Heads up @jneira and other Windows users – I'm not sure how well |
sjakobi
force-pushed
the
sjakobi/1540-atomic-cache
branch
from
4e9883e
to
feeb9c1
Compare
Fixes #1540.