{"action":"created","issue":{"url":"https://api.github.com/repos/ekmett/exceptions/issues/63","repository_url":"https://api.github.com/repos/ekmett/exceptions","labels_url":"https://api.github.com/repos/ekmett/exceptions/issues/63/labels{/name}","comments_url":"https://api.github.com/repos/ekmett/exceptions/issues/63/comments","events_url":"https://api.github.com/repos/ekmett/exceptions/issues/63/events","html_url":"https://github.com/ekmett/exceptions/issues/63","id":302375396,"number":63,"title":"regression: state updates performed while releasing are discarded","user":{"login":"gelisam","id":49000,"avatar_url":"https://avatars1.githubusercontent.com/u/49000?v=4","gravatar_id":"","url":"https://api.github.com/users/gelisam","html_url":"https://github.com/gelisam","followers_url":"https://api.github.com/users/gelisam/followers","following_url":"https://api.github.com/users/gelisam/following{/other_user}","gists_url":"https://api.github.com/users/gelisam/gists{/gist_id}","starred_url":"https://api.github.com/users/gelisam/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/gelisam/subscriptions","organizations_url":"https://api.github.com/users/gelisam/orgs","repos_url":"https://api.github.com/users/gelisam/repos","events_url":"https://api.github.com/users/gelisam/events{/privacy}","received_events_url":"https://api.github.com/users/gelisam/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":11,"created_at":"2018-03-05T16:47:03Z","updated_at":"2018-03-06T20:36:06Z","closed_at":null,"author_association":"NONE","body":"Commit 7ec3240eabc246b38ff0dc24af0be4535d7b57cf says:\r\n\r\n> we are now forced to perform some discarding of updated state and Monoid written values in the StateT, WriterT, and RWST instances. This seems inherent to making things compatible with ExceptT.\r\n\r\nbut this important semantic change is not highlighted in `CHANGELOG.markdown` nor in the documentation of `bracket`. More precisely, the `release` action does not see the state updates which have been performed within the `use` action, and the code which follows `bracket acquire release use` does not see the state updates which have been performed within `release`. An example in which discarding those state updates is problematic would be a reference-counted wrapper around `bracket` which allows nested calls to `bracket` to re-use the outer `bracket`'s already allocated resource:\r\n\r\n    {-# LANGUAGE FlexibleContexts, LambdaCase #-}\r\n    import Control.Monad.Catch\r\n    import Control.Monad.State\r\n\r\n\r\n    refCountedBracket\r\n      :: (MonadMask m, MonadState (Maybe (a, Int)) m)\r\n      => m a -> (a -> m b) -> (a -> m c) -> m c\r\n    refCountedBracket acquire release = bracket acquire' release'\r\n      where\r\n        acquire' = get >>= \\case\r\n          Nothing     -> do r <- acquire\r\n                            put (Just (r, 1))\r\n                            pure r\r\n          Just (r, n) -> do put (Just (r, n+1))\r\n                            pure r\r\n        release' _ = get >>= \\case\r\n          Nothing     -> error \"ref-counter got out of sync\"\r\n          Just (r, 1) -> do _ <- release r\r\n                            put Nothing\r\n          Just (r, n) -> put (Just (r, n-1))\r\n\r\n\r\n    data Resource = UsingResource deriving Show\r\n\r\n    withRefCountedResource\r\n      :: (MonadIO m, MonadMask m, MonadState (Maybe (Resource, Int)) m)\r\n      => (Resource -> m a) -> m a\r\n    withRefCountedResource = refCountedBracket acquire release\r\n      where\r\n        acquire = do liftIO $ putStrLn \"acquire\"\r\n                     pure UsingResource\r\n        release UsingResource = liftIO $ putStrLn \"release\"\r\n\r\nSuch an implementation works fine with exceptions-0.8.3, but causes a use-after-free with exceptions-0.9.0:\r\n\r\n    -- |\r\n    -- With exceptions-0.8.3:\r\n    --\r\n    -- >>> test\r\n    -- acquire\r\n    -- (UsingResource,UsingResource)\r\n    -- release\r\n    -- acquire\r\n    -- UsingResource\r\n    -- release\r\n    --\r\n    --\r\n    -- With exceptions-0.9.0:\r\n    --\r\n    -- >>> test\r\n    -- acquire\r\n    -- (UsingResource,UsingResource)\r\n    -- release\r\n    -- UsingResource\r\n    test :: IO ()\r\n    test = flip evalStateT Nothing $ do\r\n      withRefCountedResource $ \\r1 -> do\r\n        withRefCountedResource $ \\r2 -> do\r\n          liftIO $ print (r1, r2)\r\n      withRefCountedResource $ \\r3 -> do\r\n        liftIO $ print r3\r\n\r\nWith exceptions-0.8.3, everything goes as planned: the nested `r2` block sees that it is nested so it doesn't acquire nor release the resource, while the `r3` block sees that it isn't nested, so it re-acquires and re-releases the resource.\r\n\r\nWith exceptions-0.9.0, the situation is a lot more complicated. The `r1` block acquires the resource and initializes the reference counter to 1. Since this occurs during the `acquire` action, this will be visible to all the subsequent code. The nested `r2` block then increments the counter, but since the entirety of `r2` is within `r1`'s `use` action, the `release` action of `r1` won't see that change. The `release` action of `r2` does see that change, and thus chooses not to release the resource and to decrement the counter instead, but no other code will see this decrement. In particular, the `r1` block's `release` action sees neither `r2`'s increment nor its decrement, so as far as it's concerned, the reference counter is still 1 and so it releases the resource. It also sets the state to `Nothing`, but no other code will see that. In particular, the `r3` block sees all of the increments but none of the decrements, and so it thinks it is nested inside the `r2` block and does not re-acquire the resource. Inside `r3`, the resource is used again, but the resource has been freed already, so that's no good.\r\n\r\nIf this unfortunate behaviour really is required in order to give a `MonadMask` instance to `ExceptT`, would it be possible to change the type of the release block to `a -> IO c` instead of `a -> m c`, to make it more clear that we can't safely use non-IO effects within it?"},"comment":{"url":"https://api.github.com/repos/ekmett/exceptions/issues/comments/370919193","html_url":"https://github.com/ekmett/exceptions/issues/63#issuecomment-370919193","issue_url":"https://api.github.com/repos/ekmett/exceptions/issues/63","id":370919193,"user":{"login":"gelisam","id":49000,"avatar_url":"https://avatars1.githubusercontent.com/u/49000?v=4","gravatar_id":"","url":"https://api.github.com/users/gelisam","html_url":"https://github.com/gelisam","followers_url":"https://api.github.com/users/gelisam/followers","following_url":"https://api.github.com/users/gelisam/following{/other_user}","gists_url":"https://api.github.com/users/gelisam/gists{/gist_id}","starred_url":"https://api.github.com/users/gelisam/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/gelisam/subscriptions","organizations_url":"https://api.github.com/users/gelisam/orgs","repos_url":"https://api.github.com/users/gelisam/repos","events_url":"https://api.github.com/users/gelisam/events{/privacy}","received_events_url":"https://api.github.com/users/gelisam/received_events","type":"User","site_admin":false},"created_at":"2018-03-06T20:36:06Z","updated_at":"2018-03-06T20:36:06Z","author_association":"NONE","body":"Sure, that should work too."}}
{"actor":{"display_login":"gelisam"}}