[darcs-devel] [patch1980] split off D.R.Pristine and D.R.Traverse ... (and 4 more)

[Ganesh Sittampalam]

Ganesh Sittampalam <ganesh at earth.li> added the comment:

On 26/02/2020 07:57, Ben Franksen wrote:

> A completely different question is whether it is a good idea to first
> check for suitable conditions before running an action to avoid possible
> failures, or whether it is better to let the action fail and then handle
> that. There is (astonishingly) an almost universal consensus that the
> latter is to be preferred. This is because the former method always
> involves a race condition with outside agents: another process may
> interfere between the check and the action and invalidate the result of
> the check before the action is run. This is generally referred to as
> TOCOU (time of check, time of use). It impedes reliability and can lead
> to bugs that are very hard to reproduce.
> 
> We should /always/ let IO actions fail and then handle the failure and
> /never/ first check for suitable conditions.

If we are relying on this property to guarantee atomicity with respect
to outside agents, we'd need to check what the library we call actually
does. But in practice most IO actions we do are compound ones anyway.
Take this example:

> -- | Remove unreferenced files in the patches directory.
> cleanPatches :: Repository rt p wR wU wT -> IO ()
> cleanPatches _ = do
>     debugMessage "Cleaning out patches..."
>     hs <- (specialPatches ++) <$> listPatchesLocal PlainLayout darcsdir darcsdir
>     fs <- ifIOError [] (listDirectory patchesDirPath)
>     mapM_ (removeFileMayNotExist . (patchesDirPath </>)) (diffHashLists fs hs)

Whether or not listDirectory is atomic, removing each unwanted patch
file one by one certainly isn't. Yes, removeFileMayNotExist is itself
atomic, but by the time we get round to running it, hs might no longer
be valid if someone changed the repo, so we'd be doing the wrong thing
anyway.

Anyway, I'm ok with the current code, even if I don't think it buys us
anything.

__________________________________
Darcs bug tracker <bugs at darcs.net>
<http://bugs.darcs.net/patch1980>
__________________________________