Opened 2 years ago

Closed 21 months ago

#7787 closed bug (fixed)

modifyMVar does not restore value if callback returns error value

Reported by: joeyadams Owned by: simonmar
Priority: high Milestone: 7.8.1
Component: libraries/base Version: 7.7
Keywords: Cc: pho@…
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: Incorrect result at runtime Test Case:
Blocked By: Blocking:
Related Tickets: Differential Revisions:

Description

modifyMVar is currently implemented as follows:

modifyMVar :: MVar a -> (a -> IO (a,b)) -> IO b
modifyMVar m io =
  mask $ \restore -> do
    a      <- takeMVar m
    (a',b) <- restore (io a) `onException` putMVar m a
    putMVar m a'
    return b

The problem is that it forces the (a',b) outside of the exception handler. If forcing this throws an exception, putMVar will not be called, and a subsequent withMVar or similar will hang. Example:

> import Control.Concurrent.MVar
> mv <- newMVar 'x'
> modifyMVar mv $ \_ -> return undefined
*** Exception: Prelude.undefined
> withMVar mv print
-- hang --

Perhaps we can fix it like this:

-     (a',b) <- restore (io a) `onException` putMVar m a
+     (a',b) <- restore (io a >>= evaluate) `onException` putMVar m a

Change History (7)

comment:1 follow-up: Changed 2 years ago by ezyang

Another possibility is to replace the pattern match with an irrefutable pattern. I'm not sure which is better, though an irrefutable pattern could make laziness bugs worse.

comment:2 Changed 2 years ago by PHO

  • Cc pho@… added

comment:3 Changed 2 years ago by igloo

  • difficulty set to Unknown
  • Milestone set to 7.8.1
  • Priority changed from normal to high

comment:4 in reply to: ↑ 1 Changed 22 months ago by parcs

Replying to ezyang:

Another possibility is to replace the pattern match with an irrefutable pattern. I'm not sure which is better, though an irrefutable pattern could make laziness bugs worse.

Btw this is what atomicModifyIORef essentially does.

comment:5 Changed 21 months ago by simonmar

  • Owner set to simonmar

comment:6 Changed 21 months ago by simonmar

Fixed:

commit fd39d598e570145ac144b9cb6512c7faa3092ae1

Author: Simon Marlow <[email protected]>
Date:   Fri Jul 19 09:09:39 2013 +0100

    Fix #7787
    
    Now the pair is evaluated strictly inside the exception handler.  I
    figured this would be less likely to introduce space leaky bugs than
    using an irrefutable pattern match.

comment:7 Changed 21 months ago by simonmar

  • Resolution set to fixed
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.