Opened 13 months ago

Closed 9 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 Difficulty: Unknown
Test Case: Blocked By:
Blocking: Related Tickets:

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 13 months 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 13 months ago by PHO

  • Cc pho@… added

comment:3 Changed 12 months 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 10 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 9 months ago by simonmar

  • Owner set to simonmar

comment:6 Changed 9 months ago by simonmar

Fixed:

commit fd39d598e570145ac144b9cb6512c7faa3092ae1

Author: Simon Marlow <marlowsd@gmail.com>
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 9 months ago by simonmar

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