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?

[snoyberg]

I've written a commit to demonstrate one failure mode for directory-based file locking:

snoyberg@ffb83f9

Quoting the commit message:

Due to the nature of directory-based locking, if the process exits
before exception handlers are run, the directory is never removed and
the system remains in a "locked" state indefinitely. This can happen due
to SIGKILL or power failure, as well as a blocked thread when main
exits. This repro follows that last approach, forking a thread and then
letting main exit while that locked thread delays.

Regarding the larger issue of async exception safety here: one possibility is to implement the same technique used in safe-exceptions in the Hackage.Security.Util.Checked module, namely:

• Differentiate at the type level between sync and async exceptions using the SomeAsyncException type
• Ensure that all exceptions passed to throwIO are synchronous
• Disallow catchChecked, handleChecked, and tryChecked from catching any asynchronous exception.

Note that, AFAICT, the async exception safety issue is almost entirely orthogonal to the issue of using directory creation/removal as a form of locking.

[snoyberg]

I've opened up #202 with the async exception detection logic I described above.

[snoyberg]

I can also report that, with my patch in #202, Stack no longer has the buggy behavior described in commercialhaskell/stack#3073 (see my comment commercialhaskell/stack#3073 (comment)).