Avoid async exceptions footgun in shelly'

[adinapoli]

This PR uses catch and catches from the safe-exceptions library in a few places, notably in catches_sh, which is run as part of shelly'.

Without this commit, if a Sh computation were being run as part of an async computation (say, a snap web handler) if we had an async exception being thrown by the main handler, it would be caught by catches_sh and rethrown as a sync exception via ReThrownException, meaning that if the caller of shelly had an exception handler like try even if it came from the safe-exceptions library, this would be now be exercised because the exception got converted into a synchronous one.

This is especially problematic because it hides the real culprit; a user might be misled thinking this was an exception coming from the guts of the shelly computation, whereas it was not.

@andreasabel I know that dealing with exceptions (especially async ones) is always a bit of an headache, but does this sound sensible to you? I have verified in some upstream code that now the latter exhibit the correct behavior (i.e. the try doesn't catch it, and the exception is correctly propagated up the ladder).

[adinapoli]

Build profile: -w ghc-9.14.0.20250819 -O1
In order, the following will be built (use -v for more details):

• async-2.2.6 (lib) (requires download & build)
• monad-control-1.0.0.2 (lib:monad-control) (requires download & build)
• lifted-base-0.2.3.12 (lib) (requires download & build)
• enclosed-exceptions-1.0.3 (lib) (requires download & build)

Uhm, CI seems to be unrelated to my changes, at least from a quick glance?

[andreasabel]

Uhm, CI seems to be unrelated to my changes, at least from a quick glance?

Yes, Haskell CI for 9.14 is currently not possible:

• Bump CI to GHC 9.14.1 #236

[andreasabel]

@adinapoli Thanks for the PR.
Do you think it is possible to add a test that demonstrates the difference between "before" and "after"?

[adinapoli]

@adinapoli Thanks for the PR. Do you think it is possible to add a test that demonstrates the difference between "before" and "after"?

@andreasabel of course :) Easter holidays might get in the way, but it will come.

[adinapoli]

@andreasabel As promised, here is the reproducer. I have a full, dead-simple cabal project stashed away if you want me to put it online somewhere for you to experiment. Either way, consider the following:

{-# LANGUAGE NumericUnderscores #-}
module Main where

import Control.Concurrent       (threadDelay)
import qualified Control.Exception.Safe as Safe
import System.Timeout (timeout)
import Shelly

doWork :: IO (Either String ())
doWork = do
  res <- Safe.tryAny $ shelly $ silently $ liftIO $ threadDelay 5_000_000
  case res of
    Left err -> pure $ Left $ Safe.displayException err
    Right _  -> pure $ Right ()

main :: IO ()
main = do
  -- Run a long-lived action in a separate thread, simulating a web
  -- request handler that shells out during processing.
  threadResult <- timeout 1_000_000 doWork
  case threadResult of
    Nothing -> putStrLn $ "OK: timeout hit! shelly didn't catch the async Timeout exception."
    Just res -> case res of
      Left err -> putStrLn $ "BUG: shelly caught async exception: " <> err
      Right () -> putStrLn $ "OK: we returned. (never exercised in this reproducer!)"

This replicates what happens in a typical backend server application (or, at least, what happens is my server) we have a main thread spawning subthreads to serve requests, and we have a timeout that throws an async exception to kill the thread to avoid Slowloris attacks.

Here we are conveniently using the timeout function from System.Timeout, which throws an async exception to the green thread spawned if the timeout is hit, pretty much replicating what happens in (say) a Snap server.

The conjured scenario is simple: we spawn an async action and we want to wait for its result and inspect it. However, our subthread will never finish because it's too slow, triggering the timeout.

I would argue that the correct behaviour for the system is that the Timeout exception is thrown and caught by who threw it in the first place, i.e. the main thread, and thus by contract, the timeout should return Nothing.

However, without this PR, look at what happens if I run this code:

BUG: shelly caught async exception:
Ran commands:

Exception: <<timeout>>

What happened there? Well, the main thread threw a Timeout exception to the green thread, but it was instead caught by the shelly internal catches, rethrown as a synchronous exception and now all of the sudden we are getting a result, even thought by contract we shouldn't, because the timeout was hit.

If I switch to use my version of Shelly, as per this commit, we get:

OK: timeout hit! shelly didn't catch the async Timeout exception.

My claim is that this abides to the principle of least surprise -- if sometimes takes more than it should, I would be expecting the timeout function to return Nothing, not to return something that, upon inspection, reveals that something was actually timing out!

I hope this helps! 😁

[andreasabel]

Thanks! Is there a way to add this reproducer to the testsuite?

You could release this as 1.13.
I can add you on Hackage but not to this repo as it is in a personal account.
@gregwebs Would you add @adinapoli as committer here?

[adinapoli]

Thanks! Is there a way to add this reproducer to the testsuite?

I guess we could add this pretty much unaltered in a unit test, given that it doesn't have any extra dependencies and we can assert that we should always get Nothing as the final result (where the threadDelay figures we can lower them down to a suitable value to not inflate the tests running times too much).

[adinapoli]

@andreasabel Just to be on the same page, are you waiting for me on doing anything specific in order to progress this further? I have stumbling into this problem more and more in projects at work, and it would be great to have this at least merged here, to not have to depend on a branch and not on the "official" code 😉

[andreasabel]

@adinapoli : Yes, basically, I do not want to take responsibility for this change (there is no test). I do not oppose the change, but I am not interested to develop this package further, so I would be grateful if you took over.

@gregwebs Did you have time to think about this? It might be better to move this package away from a personal account to an organization so that onboarding of new maintainers is easier.

[adinapoli]

Yes, basically, I do not want to take responsibility for this change (there is no test). I do not oppose the change, but I am not interested to develop this package further, so I would be grateful if you took over.

@andreasabel I'm happy to take over maintainership of this library, given that we (Well-Typed) use it extensively for a client (IRIS Connect), which is willing to cover the time spent for improving it -- I will leave you and Greg to sort out the details, but I'm happy to deal with versioning, merging/reviewing new code, releasing things to Hackage etc as you see fit.

[andreasabel]

We need to get hold of @gregwebs to (best-case) transfer this repo to an organisation.
I don't seem to have an email adress of him.

[gregwebs]

I definitely like using organizations. Let me know how I can help with long-term maintainership.

[andreasabel]

There are two main options:

1. Make a dedicated organization for "shelly". I did such e.g. for package github and package blaze-builder. An organization can have several owners, so one can pass on maintainership with full control for the new maintainer.
2. Use an existing organization dedicated to maintainership, e.g. https://github.com/haskell-pkg-janitors or https://github.com/haskell-github-trust .

If you want to use haskell-pkg-janitors , I am owner there and can give out necessary rights (including admin rights for a repo that one cannot give when the repo is on a personal account).

@gregwebs @adinapoli What would be your preference?

[adinapoli]

@andreasabel what you and Greg find easier -- at some point I've found myself involved in the haskell-pkg-janitors for some other packages I have now forgot which one it was, but that routes certainly work for me, if that's simpler.

[andreasabel]

@adinapoli I invited you to haskell-pkg-janitors .
@gregwebs I tried to invite you as well, but github did not find you when I tried "gregwebs" or "Greg Weber". Are there some restrictions set up with your account? Last option would be try to invite you per email address.

[gregwebs]

No, I did not set any restrictions. You can find my email address in the early commits of the repo.

[adinapoli]

@adinapoli I invited you to haskell-pkg-janitors .

@andreasabel I have accepted!

[andreasabel]

@gregwebs I invited you per email now.
You should be able to transfer this repo to haskell-pkg-janitors once you accept.

[andreasabel]

@gregwebs I see you are a member now. You should be able to transfer the repo to the organization now. (I don't think I can do this.)