Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

on Windows, copyFile makes empty source and destination files when the source file is absent #177

Closed
hsenag opened this issue May 19, 2024 · 8 comments
Labels
regression in 1.3.8.0 type: a-bug The described behavior is not working as intended.

Comments

@hsenag
Copy link
Member

hsenag commented May 19, 2024

on Windows,

copyFile nonexistent shouldnotexist

succeeds and leaves 0-byte files for both nonexistent and shouldnotexist.

I would expect it to throw an exception and not create either source or destination file, which is what happens on Linux and MacOS.

The behaviour seems to have started in directory-1.3.8.0 and I bisected it to 78b3e59. I'm fairly sure it's directory because I did things
like holding Win32 and trying with a range of GHCs. With default versions we only noticed it starting with GHC 9.6, but I get the same
behaviour with GHC 9.4.8+Win32 2.13.3.0+directory 1.3.8.0 (for example).

I used this test program to track down where the problem started:

module Main where

import Control.Exception
import System.Directory
import System.IO.Error (isDoesNotExistError)

main :: IO ()
main = do
  removeFile "nonexistent.source" `catch` (\e -> if isDoesNotExistError e then return () else throwIO e)
  removeFile "shouldnotexist.target" `catch` (\e -> if isDoesNotExistError e then return () else throwIO e)
  (copyFile "nonexistent.source" "shouldnotexist.target" >> putStrLn "BAD: copyFile did not throw") `catch` (\e -> if isDoesNotExistError e then putStrLn "OK: copyFile threw" else throwIO e)
  doesFileExist "nonexistent.source" >>= \b -> if b then putStrLn "BAD: nonexistent.source exists" else putStrLn "OK: nonexistent.source does not exist"
  doesFileExist "shouldnotexist.target" >>= \b -> if b then putStrLn "BAD: shouldnotexist.target exists" else putStrLn "OK: shouldnotexist.target does not exist"
  removeFile "nonexistent.source" `catch` (\e -> if isDoesNotExistError e then return () else throwIO e)
  removeFile "shouldnotexist.target" `catch` (\e -> if isDoesNotExistError e then return () else throwIO e)
  return ()

ref: https://bugs.darcs.net/issue2721

@hasufell
Copy link
Member

I can confirm. The relevant code is here:

-- | A helper function useful for replacing files in an atomic manner. The
-- function creates a temporary file in the directory of the destination file,
-- opens it, performs the main action with its handle, closes it, performs the
-- post-action with its path, and finally replaces the destination file with
-- the temporary file. If an error occurs during any step of this process,
-- the temporary file is removed and the destination file remains untouched.
withReplacementFile :: OsPath -- ^ Destination file
-> (OsPath -> IO ()) -- ^ Post-action
-> (Handle -> IO a) -- ^ Main action
-> IO a
withReplacementFile path postAction action =
(`ioeAddLocation` "withReplacementFile") `modifyIOError` do
mask $ \ restore -> do
-- TODO: AFPP doesn't support openBinaryTempFile yet,
-- so we have to use this (sad) workaround
-- (on unix, converts using filesystem encoding, on windows
-- converts with UTF-16LE)
d <- decodeFS (takeDirectory path)
(tmpFPath', hTmp) <- openBinaryTempFile d ".copyFile.tmp"
tmpFPath <- encodeFS tmpFPath'
(`onException` ignoreIOExceptions (removeFile tmpFPath)) $ do
r <- (`onException` ignoreIOExceptions (hClose hTmp)) $ do
restore (action hTmp)
hClose hTmp
restore (postAction tmpFPath)
renameFile tmpFPath path
pure r

@hasufell
Copy link
Member

Fix is here: #178

However, I suggest @Rufflewind that we consider to use the package file-io for this in the future so that we can consolidate such low-level functionality. Having complicated Win32 code spread around different packages is problematic.

The openExistingFile function from that package does not suffer from this issue.

Rufflewind pushed a commit to Rufflewind/directory that referenced this issue May 20, 2024
Rufflewind added a commit to Rufflewind/directory that referenced this issue May 20, 2024
Rufflewind added a commit to Rufflewind/directory that referenced this issue May 20, 2024
@Rufflewind
Copy link
Member

Released: https://hackage.haskell.org/package/directory-1.3.8.5

@hasufell
Copy link
Member

@bgamari do you have visibility which GHC versions ship with this bug?

@hsenag
Copy link
Member Author

hsenag commented May 20, 2024

Thanks for the quick fix/release!

@Rufflewind
Copy link
Member

This affects directory 1.3.8.0 through 1.3.8.4, which maps to GHC 9.6.1 through 9.10.1 according to https://gitlab.haskell.org/ghc/ghc/-/wikis/commentary/libraries/version-history

@Rufflewind Rufflewind added the type: a-bug The described behavior is not working as intended. label May 20, 2024
@bgamari
Copy link
Contributor

bgamari commented May 20, 2024

I have opened GHC#24843 to ensure that this bump is performed in future minor releases.

@andreasabel
Copy link
Member

This affects directory 1.3.8.0 through 1.3.8.4,

Would be good to add this info in the release notes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression in 1.3.8.0 type: a-bug The described behavior is not working as intended.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants