writeFileAtomic retry strategy could be improved with threadDelay

[Bodigrim]

cabal/Cabal-syntax/src/Distribution/Utils/Generic.hs

Lines 184 to 200 in c2d8353

	writeFileAtomic :: FilePath -> LBS.ByteString -> IO ()
	writeFileAtomic targetPath content = do
	let targetFile = takeFileName targetPath
	tmpDir <- getTemporaryDirectory
	Exception.bracketOnError
	(openBinaryTempFileWithDefaultPermissions tmpDir $ targetFile <.> "tmp")
	(\(tmpPath, handle) -> hClose handle >> removeFile tmpPath)
	( \(tmpPath, handle) -> do
	LBS.hPut handle content
	hClose handle
	Exception.catch
	(renameFile tmpPath targetPath)
	( \(_ :: Exception.SomeException) -> do
	copyFile tmpPath targetPath
	removeFile tmpPath
	)
	)

So if renameFile failed we immediately try copyFile followed by removeFile. My experience however is that this is not helpful, at least on Windows. The most common reason for renameFile to fail is because another process (typically, an antivirus) opened the source file for reading. If this is the case then copyFile is likely to be too short delay for removeFile to succeed, because the source file remains locked by antivirus.

To make it worse, copyFile from directory internally makes its own attempt at atomicity, creating a temporary file and renaming it, which triggers the antivirus yet again.

In short, if renameFile fails then there is no reason to expect copyFile / removeFile to succeed.

I'd suggest to scrap copyFile / removeFile catch: just keep retrying renameFile after threadDelay 10000 a few times more.

CC @GulinSS who was looking at writeFileAtomic recently.

[GulinSS]

Here I suspect that the issue with c_openat is due of fact we do not use system-defined O_*. So, they are just hardcoded.

When O_CREAT check happened at ghc js-backend side, it does not recognize O_CREAT and fails when Cabal's Setup.hs compiled to js is running.

Looking around for a solution. Probably unix package should be patched to use Base.o_CREAT vs (#const O_CREAT) because o_CREAT can have a different value at stage1. Not system's one.

[Mikolaj]

Do we have a confirmation it's a problem in unix? Anything in cabal that should be fixed/improved, too?

[GulinSS]

Unix package provides primitives which are used by cabal. Cabal makes assumptions about "atomicity" which really have no strong reasons for file movements. So, if cabal wants to continue using mv pattern with cp+rm it should take into consideration that other platforms like Windows have special cases which @Bodigrim said at the original message.

So , shortly say, yes, mv with cp+rm is cabal stuff not unix, and cabal is a right place where probably need take into account other platforms like Windows where additional delays might be useful.

[zlonast]

Will we be happy with an atomic replacement of the file contents?

In Windows, the ReplaceFile operation is atomic on move.