Broken beyond repair: Control.Concurrent.SampleVar
Hello all, The SampleVar module in base is not exception safe. I believe that there is no way to fix this module to be exception safe while retaining the current behavior. The problem with the current behavior is that the writeSampleVar pretends to know how many blocked reader threads are waiting on a value. In reality these blocked threads may have been killed. When writeSampleVar sees blocked threads it does a blocking putMVar and then decrements the blocked reader thread count. If any two of the blocked reader threads ever die then eventually you get a blocked writer. I can find no efficient way to retain the current behavior in the presence of exceptions. The logic of the current behavior is that the blocked readers mean that the previously written value should not be discarded, the previous value belongs to the next blocked reader. I propose that SampleVar be either removed, or replaced with a slightly different exception safe version. I propose not considering the previously written value to belong to a blocked reader, and to replace it with the new value. I will open a ticket about this when I get more time. -- Chris
Hello ChrisK, Sunday, April 12, 2009, 4:18:44 PM, you wrote:
The problem with the current behavior is that the writeSampleVar pretends to know how many blocked reader threads are waiting on a value. In reality these blocked threads may have been killed.
may be it's possible to add exception handling to the write operation? -- Best regards, Bulat mailto:Bulat.Ziganshin@gmail.com
Bulat Ziganshin wrote:
Hello ChrisK,
Sunday, April 12, 2009, 4:18:44 PM, you wrote:
The problem with the current behavior is that the writeSampleVar pretends to know how many blocked reader threads are waiting on a value. In reality these blocked threads may have been killed.
may be it's possible to add exception handling to the write operation?
more obviously importantly (I think, from that description,) is exception handling around the blocking read operation -- after all, even thread-killing involves an exception.
Bulat Ziganshin wrote:
Hello ChrisK,
Sunday, April 12, 2009, 4:18:44 PM, you wrote:
The problem with the current behavior is that the writeSampleVar pretends to know how many blocked reader threads are waiting on a value. In reality these blocked threads may have been killed.
may be it's possible to add exception handling to the write operation?
A perfect fix would leave SampleVar's full behavior unchanged when there are no exceptions. This perfect fix does not exist. I claim it is possible to create a fix that leaves the documented behavior unchanged, but changes some of the actual detailed behavior. One such fix is up at http://haskell.org/haskellwiki/SafeConcurrent#SampleVar -- Chris
Why not
import Control.Applicative import Control.Concurrent.MVar import Control.Exception
data SampleVar a = SV {svLock :: MVar (), svData :: MVar a}
newEmptySampleVar :: IO (SampleVar a) newEmptySampleVar = SV <$> newMVar () <$> newEmptyMVar
newSampleVar :: a -> IO (SampleVar a) newSampleVar x = SV <$> newMVar () <$> newMVar x
emptySampleVar :: SampleVar a -> IO () emptySampleVar = (>> return ()) . tryTakeMVar . svData
readSampleVar :: SampleVar a -> IO a readSampleVar = takeMVar . svData
isEmptySampleVar :: SampleVar a -> IO Bool isEmptySampleVar = isEmptyMVar . svData
writeSampleVar :: SampleVar a -> a -> IO () writeSampleVar s x = block $ withMVar (svLock s) $ const $ do tryTakeMVar (svData s) putMVar (svData s) x
? -- Felipe.
Felipe Lessa wrote:
writeSampleVar :: SampleVar a -> a -> IO () writeSampleVar s x = block $ withMVar (svLock s) $ const $ do tryTakeMVar (svData s) putMVar (svData s) x
That is obvious, but 'block $' is not atomic. Threads 1 and 2 get past the tryTakeMVar. Then thread 1 succeeds with putMVar and thread 2 blocks. Thread 2 is not supposed to block, so this is a failure to agree with the specification.
On Sun, Apr 12, 2009 at 05:12:15PM +0100, Chris Kuklewicz wrote:
Felipe Lessa wrote:
writeSampleVar :: SampleVar a -> a -> IO () writeSampleVar s x = block $ withMVar (svLock s) $ const $ do tryTakeMVar (svData s) putMVar (svData s) x
That is obvious, but 'block $' is not atomic.
Threads 1 and 2 get past the tryTakeMVar.
That's why there is a "withMVar (svLock s)", the "const $ do" part is executed serially. It is impossible to have two threads past the "tryTakeMVar".
Then thread 1 succeeds with putMVar and thread 2 blocks.
Thread 2 is not supposed to block, so this is a failure to agree with the specification.
Well, it doesn't block. In the worst case I can imagine a thread could be killed after the 'tryTakeMVar', leaving the 'SampleVar' empty, but I guess the 'block' would prevent that. -- Felipe.
and I am dumb, sorry. Yes, you are right. In fact the code I had posted on the wiki at http://haskell.org/haskellwiki/SafeConcurrent#SampleVar is
writeSampleVar :: SampleVar a -> a -> IO () writeSampleVar svar value = do withMVar (lockedStore svar) $ \ store -> do _ <- tryTakeMVar store putMVar store value
which is functionally identical to yours. I should not write so hastily on my way out the door... Felipe Lessa wrote:
On Sun, Apr 12, 2009 at 05:12:15PM +0100, Chris Kuklewicz wrote:
Felipe Lessa wrote:
writeSampleVar :: SampleVar a -> a -> IO () writeSampleVar s x = block $ withMVar (svLock s) $ const $ do tryTakeMVar (svData s) putMVar (svData s) x That is obvious, but 'block $' is not atomic.
Threads 1 and 2 get past the tryTakeMVar.
That's why there is a "withMVar (svLock s)", the "const $ do" part is executed serially. It is impossible to have two threads past the "tryTakeMVar".
Then thread 1 succeeds with putMVar and thread 2 blocks.
Thread 2 is not supposed to block, so this is a failure to agree with the specification.
Well, it doesn't block. In the worst case I can imagine a thread could be killed after the 'tryTakeMVar', leaving the 'SampleVar' empty, but I guess the 'block' would prevent that.
-- Felipe.
Felipe Lessa wrote:
writeSampleVar :: SampleVar a -> a -> IO () writeSampleVar s x = block $ withMVar (svLock s) $ const $ do tryTakeMVar (svData s) putMVar (svData s) x
withMVar is a blocking operation, thus interruptible despite 'block', I believe... perhaps moving the block inward might more clearly specify what happens (and be *at least* as correct, maybe more correct?) : writeSampleVar s x = withMVar (svLock s) $ const $ block $ do tryTakeMVar (svData s) --nonblocking, thus not exception-interruptible putMVar (svData s) x --because the lock is taken and the MVar emptied, -- this is guaranteed to succeed and not to block -Isaac
Isaac Dupree wrote:
Felipe Lessa wrote:
writeSampleVar :: SampleVar a -> a -> IO () writeSampleVar s x = block $ withMVar (svLock s) $ const $ do tryTakeMVar (svData s) putMVar (svData s) x
withMVar is a blocking operation, thus interruptible despite 'block', I believe... perhaps moving the block inward might more clearly specify what happens (and be *at least* as correct, maybe more correct?) :
writeSampleVar s x = withMVar (svLock s) $ const $ block $ do tryTakeMVar (svData s) --nonblocking, thus not exception-interruptible putMVar (svData s) x --because the lock is taken and the MVar emptied, -- this is guaranteed to succeed and not to block
As far as I can see, I agree: "block" and "withMVar/modifyMVar/modifyMVar_" mostly commute. So it is largely a matter of taste. mostly mostly By blocking second, you allow interruptions between withMVar succeeding and the block taking effect. Also, the user could potentially wrap the call in an outer "block". By putting block first: if the lock is contested then once the withMVar succeeds the operation is guaranteed to commit; and if the lock is not contested the whole operation inside block will always commit. In point of fact, the error handler set up by the withMVar is superfluous and tighter code be written (though less clear) My taste is the latter: by moving the "block" to the outermost level it makes the code easier to reason about (good for designer) and it means the caller never has to think about wrapping it an outer "block" (good for the user). The tighter code version of http://haskell.org/haskellwiki/SafeConcurrent#SampleVar :
writeSampleVar svar value = block $ do store <- takeMVar (lockedStore svar) _ <- tryTakeMVar store putMVar store x putMVar (lockedStore svar) store
-- Chris
participants (5)
-
Bulat Ziganshin -
Chris Kuklewicz -
ChrisK
-
Felipe Lessa -
Isaac Dupree