A couple spots async exceptions aren't handled gracefully

[mgsloan]

Based on commercialhaskell/stack#3073 , I suspected hackage-security wasn't handling async exceptions quite right. Indeed, a search for SomeException led me to find the following spots where SomeException gets caught and not rethrown:

Relevant to the linked issue:

hackage-security/hackage-security/src/Hackage/Security/Client/Repository/Remote.hs

Line 278 in 226e1e4

catchChecked (select m callback) $ \ex -> do

Part of hackage-repo-tool:

hackage-security/hackage-repo-tool/src/Hackage/Security/RepoTool/Util/IO.hs

Line 84 in 226e1e4

handle (throwIO . TarGzError (prettyTargetPath' opts pathTarGz)) $ do

I also noticed that the UpdateFailed exception constructor is never used -

hackage-security/hackage-security/src/Hackage/Security/Client/Repository.hs

Line 336 in 226e1e4

| UpdateFailed SomeException

[dcoutts]

@mgsloan thanks.

@edsko looking at that use of catchChecked, I don't see how the Throws e => constraint is resolved. There doesn't seem to be any instance around.

[edsko]

@edsko looking at that use of catchChecked, I don't see how the Throws e => constraint is resolved. There doesn't seem to be any instance around.

That's the whole point. https://www.well-typed.com/blog/2015/07/checked-exceptions/ .

[hvr]

fwiw, I'd suggest adding a link to that blogpost to the Hackage.Security.Util.Checked module (which provides catchChecked) as the concept is really a little bit non-obvious to the non-initiated... :-)

[mgsloan]

It may also be good to have it wait for the lock to become available - commercialhaskell/stack#3055 (comment)

[domenkozar]

Could this be the case why commercialhaskell/stack#3055 happens even with just ctrl-c (sigint/sigterm)? I've seen it happen on CI, where I'd be surprised anything is calling SIGKILL.

[domenkozar]

Managed to reproduce it with:

$ rm -rf ~/.cabal
$ cabal update
# wait 15s
ctrl-c

$ cabal update
Downloading the latest package list from hackage.haskell.org
/home/ielectric/.cabal/packages/hackage.haskell.org/hackage-security-lock: createDirectory: already exists (File exists)

[nh2]

I think this is because of

hackage-security/hackage-security/src/Hackage/Security/Util/IO.hs

Lines 33 to 48 in 71a24d6

	-- | Attempt to create a filesystem lock in the specified directory
	--
	-- Given a file @/path/to@, we do this by attempting to create the directory
	-- @//path/to/hackage-security-lock@, and deleting the directory again
	-- afterwards. Creating a directory that already exists will throw an exception
	-- on most OSs (certainly Linux, OSX and Windows) and is a reasonably common way
	-- to implement a lock file.
	withDirLock :: Path Absolute -> IO a -> IO a
	withDirLock dir = bracket_ takeLock releaseLock
	where
	lock :: Path Absolute
	lock = dir </> fragment "hackage-security-lock"
	takeLock, releaseLock :: IO ()
	takeLock = createDirectory lock
	releaseLock = removeDirectory lock

So this seems to just create a plain directory to implement a lock. Any form of createDirectory >> removeDirectory as done there cannot survive SIGKILL or computer crashes. I would have expected this to be implemented with flock().

[hvr]

@nh2 unfortunately flock() isn't portable (a problem we're having trouble with already in cabal; but I'm working on something to address that; once I'm done we can use it hackage-security too)

[domenkozar]

https://hackage.haskell.org/package/filelock-0.1.1.2/docs/System-FileLock.html works well on Windows as long as you don't use lock paths for anything.

[hvr]

@domenkozar I'm aware of the prior art (and there's a lot of history regarding file/record locking in Unix...) :-)

[chrisdone]

@nh2 unfortunately flock() isn't portable (a problem we're having trouble with already in cabal; but I'm working on something to address that; once I'm done we can use it hackage-security too)

@domenkozar I'm aware of the prior art :-)

So what are you working on to address this?

[domenkozar]

@domenkozar I'm aware of the prior art :-)

Ah sorry, I assume you don't due to your previous comment. I guess my implicit question was if we can use filelock to solve this properly and what would be the cons (besides obvious extra dependency) to fix this bug?