Broken beyond repair: Control.Concurrent.SampleVar

[Chris Kuklewicz]

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