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

resourceToIO doesn't protect against IO exceptions thrown by interpreters run after it #268

Open
KingoftheHomeless opened this issue Oct 31, 2019 · 4 comments
Milestone

Comments

@KingoftheHomeless
Copy link
Collaborator

KingoftheHomeless commented Oct 31, 2019

I'm exploring a rework on Resource's semantics, and I discovered this issue while working on it.

For example:

import qualified Control.Monad.Fail as Fail

bad1 :: IO ()
bad1 =
    runM
  . failToEmbed @IO
  . resourceToIO
  . traceToIO
  $ do
    bracket
      (trace "alloc")
      (\_ -> trace "dealloc")
      (\_ -> trace "use" >> Fail.fail "exception")
> bad1
alloc
use
*** Exception: user error (exception)

It also doesn't protect from IO exceptions thrown via embedFinal (and thus it doesn't protect against the exceptions that errorToIOFinal throws):

bad2 :: IO ()
bad2 =
    runFinal
  . embedToFinal @IO
  . resourceToIO
  . traceToIO
  $ do
    bracket
      (trace "alloc")
      (\_ -> trace "dealloc")
      (\_ -> trace "use" >> embedFinal @IO (Fail.fail "exception"))

Fixing these would require introducing a Final IO constraint to withLowerToIO.
This would let us do the following:

  1. The interpreter thread of withLowerToIO would be able to lift and apply Control.Exception.try to the requests it gets. If it catches an exception, then it sends that back to the querying thread, which will then throw that exception within that thread. This way, brackets within the querying thread will be able to react to the exception.
  2. runViaForklift would be able to intercept withWeavingToFinal and interpret them in the forked thread, rather than shipping them off to the interpreter thread.

1 is the big thing here; 2's just a bonus.

In my Resource rework, resourceToIO would also benefit from having access to Final IO; I'll put up a draft of it soon within a week.

The downside is adding Final IO as a constraint to withLowerToIO (we may want to keep the Embed constraint so that runViaForklift may intercept it.)

KingoftheHomeless added a commit to KingoftheHomeless/polysemy that referenced this issue Oct 31, 2019
@isovector
Copy link
Member

This strikes me as the expected behavior --- it's the local/global semantics thing all over again, right? Resource will (/should?) only protect you against exceptions that are more local than it.

@KingoftheHomeless
Copy link
Collaborator Author

Depends on your perspective. IMO, Resource should always protect against IO exceptions when possible.

Anyway, even with the viewpoint that bracket should only handle IO exceptions of effects already interpreted, you still run into issues when dealing with higher-order effects:

data HO m a where
  HO :: m a -> HO m a

makeSem ''HO

runHO :: Sem (HO ': r) a -> Sem r a
runHO = interpretH $ \(HO m) -> runT m >>= raise . runHO

bad :: IO ()
bad =
    runM
  . traceToIO
  . runHO
  . resourceToIO
  . failToEmbed @IO
  $ do
    bracket
      (trace "alloc")
      (\_ -> trace "dealloc")
      (\_ -> hO $ trace "use" >> Fail.fail "bad")
> bad
alloc
use
*** Exception: user error (bad)

@isovector
Copy link
Member

My feeling is that there are very few effects you'd ever want to eliminate after Resource. Maybe State or Async? Am I missing something here?

@isovector
Copy link
Member

I suppose there's some argument in using it as part of an interpretation of some other effect. Which would indeed expect to not crash. I'm OK adding Final IO to withLowerToIO if that would fix this.

@KingoftheHomeless KingoftheHomeless mentioned this issue Jan 18, 2020
10 tasks
@KingoftheHomeless KingoftheHomeless added this to the v2 milestone Jan 18, 2020
@isovector isovector mentioned this issue Oct 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants