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

Bracket aquire function gets killed #170

Closed
felixSchl opened this issue Feb 24, 2019 · 3 comments
Closed

Bracket aquire function gets killed #170

felixSchl opened this issue Feb 24, 2019 · 3 comments

Comments

@felixSchl
Copy link

felixSchl commented Feb 24, 2019

I am experiencing some very odd behavior, to say the least. Unfortunately, I am of yet unable to reproduce this issue in a small test case, but I am still working on it so I'll have to just report the symptoms for the time being.

We are observing the acquire part of a bracket getting killed part way through it and the clean up handler running. Because the acquire handler had not finished running, the clean up handler gets an "undefined" value passed in, causing all sorts of trouble.

Hopefully I can recreate this in a small test case, but the outlook is rather grim.

EDIT: Using the code from #169 the error goes away, but the fiber never gets killed at all.

@natefaubion
Copy link
Collaborator

natefaubion commented Feb 25, 2019

Note, that all #169 does is change where async scheduling occurs. Async boundaries get put into a scheduler to ensure fairness. Currently, async scheduling happens when an async action resolves. This means that the async effect will be run promptly, but once the callback is invoked, it won't necessarily start evaluating the next part of the Aff. This can be problematic in practice, because even though someone has called the callback , the fiber is at the mercy of the scheduler, and can still be killed. This can cause all sorts of very subtle bugs and ultimately breaks a useful level of reasoning (I called the callback to complete, but it still got killed?). That PR switches it so that a fiber gets scheduled before the effect is run, and resolution is immediate. This can definitely change the order of effect evaluation. If code has been written that relies an a specific ordering of effects among forked fibers, then it will very likely break.

@felixSchl
Copy link
Author

felixSchl commented Mar 21, 2019

I have managed to reproduce the problem:

module Test.Main
  ( main
  ) where

import Prelude

import Control.Monad.Rec.Class (forever)
import Data.Time.Duration (Milliseconds(..))
import Debug.Trace (traceM)
import Effect (Effect)
import Effect.Aff (delay, forkAff, generalBracket, killFiber, launchAff_)
import Effect.Exception as Error

main :: Effect Unit
main = launchAff_ do
    f <- forkAff $
      generalBracket
        (do
            traceM "a"
            traceM =<<
              generalBracket
                (pure unit)
                { killed: \_ _ -> pure unit
                , failed: \_ _ -> pure unit
                , completed: \_ _ -> pure unit
                }
                (\_ -> delay $ 500.0 # Milliseconds)
            traceM "b"
        )
        { killed: \_ _ -> pure unit
        , failed: \_ _ -> pure unit
        , completed: \_ _ -> pure unit
        }
        (\_ -> forever $ delay $ 7500.0 # Milliseconds)

    delay $ 10.0 # Milliseconds
    traceM "crashing f..."
    killFiber (Error.error "crash") f

    traceM "waiting..."
    delay $ 1000.0 # Milliseconds
    traceM "done"

this test produces the following output:

'a'
'crashing...'
'waiting...'
'done'

note the absence of 'b' in the output

@natefaubion
Copy link
Collaborator

natefaubion commented Mar 23, 2019

Here's a reduced regression test:

test_regression_bracket_kill_mask  Aff Unit
test_regression_bracket_kill_mask = assert "regression/kill-bracket-mask" do
  ref ← newRef []
  let
    action s =
      void $ modifyRef ref (_ <> [ s ])
  fiber ← forkAff do
    bracket
      do
        action "a"
        bracket
          (pure unit)
          (const (pure unit))
          (\_ -> delay (Milliseconds 10.0))
        action "b"
      (const (pure unit))
      (\_ -> delay (Milliseconds 10.0))
  delay (Milliseconds 5.0)
  killFiber (error "nope") fiber
  readRef ref <#> eq
    [ "a"
    , "b"
    ]

natefaubion added a commit to natefaubion/purescript-aff that referenced this issue Mar 23, 2019
Currently when we check interrupt status, we are not considering the
bracket masking status. When we enter a masked state (`bracketCount > 0`),
it should be impossible for an interrupt to terminate evaluation, but
without the bracket check in CATCH, RESUME, and RELEASE, this is
violated.

*   In a nested async generalBracket, the wrong handler is enqueued.
Since we are in a masked state, it should be impossible for the `killed`
branch to be invoked. However, if an interrupt occurs it is currently
_always_ invoking `killed` when the branch is actually `completed`.
*   In a nested bracket, an interrupt will terminate evaluation of the
inner bracket even though we are in a masked state. This can also happen
with an inner `catch` in a masked state.

We need to always consider the masked state (`bracketCount > 0`) when we
discriminate the interrupt state.

Fixes purescript-contrib#170
natefaubion added a commit that referenced this issue Mar 29, 2019
Currently when we check interrupt status, we are not considering the
bracket masking status. When we enter a masked state (`bracketCount > 0`),
it should be impossible for an interrupt to terminate evaluation, but
without the bracket check in CATCH, RESUME, and RELEASE, this is
violated.

*   In a nested async generalBracket, the wrong handler is enqueued.
Since we are in a masked state, it should be impossible for the `killed`
branch to be invoked. However, if an interrupt occurs it is currently
_always_ invoking `killed` when the branch is actually `completed`.
*   In a nested bracket, an interrupt will terminate evaluation of the
inner bracket even though we are in a masked state. This can also happen
with an inner `catch` in a masked state.

We need to always consider the masked state (`bracketCount > 0`) when we
discriminate the interrupt state.

Fixes #170
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants