[GHC] #7719: System.Timeout.timeout may leak <<timeout>> exceptions
#7719: System.Timeout.timeout may leak <<timeout>> exceptions -------------------------+-------------------------------------------------- Reporter: int-e | Owner: Type: bug | Status: new Priority: normal | Component: libraries/base Version: 7.7 | Keywords: Os: Linux | Architecture: x86_64 (amd64) Failure: None/Unknown | Blockedby: Blocking: | Related: -------------------------+-------------------------------------------------- The current implementation of {{{timeout}}} can leak {{{Timeout}}} exceptions if an asynchronous exception is delivered at the wrong time. The most worrysome such case is another {{{Timeout}}} exception raised by surrounding call to {{{timeout}}}. The following program reproduces the issue reliably for me: {{{ import System.Timeout import Control.Monad import Control.Concurrent t d = timeout d $ timeout d $ timeout d $ timeout d $ timeout d $ timeout (10^9) $ threadDelay 100 main = forever $ mapM_ t [1..200] -- > ghc -O2 -rtsopts -threaded --make TT.hs -- > ./TT +RTS -N1 -- TT: <<timeout>> }}} Context: A recent thread on {{{ghc-users}}}, starting with http://www.haskell.org/pipermail/glasgow-haskell- users/2013-February/023811.html, got me thinking about corner cases of timeout handling again. -- Ticket URL: http://hackage.haskell.org/trac/ghc/ticket/7719 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#7719: System.Timeout.timeout may leak <<timeout>> exceptions -------------------------+-------------------------------------------------- Reporter: int-e | Owner: Type: bug | Status: new Priority: normal | Component: libraries/base Version: 7.7 | Keywords: Os: Linux | Architecture: x86_64 (amd64) Failure: None/Unknown | Blockedby: Blocking: | Related: -------------------------+-------------------------------------------------- Comment(by int-e): A possible scenario for two nested threads on a single capability is the following, based on the fact that {{{threadDelay}}} masks exceptions while using {{{takeMVar}}} to wait for a timeout. {{{ A = timeout n (timeout m (return ()) M = TimerManager Ti = timer threads A: outer timeout ... set up T1 A: inner timeout ... set up T2 A: threadDelay (insert into M's queue, block on MVar) T1: threadDelay (insert into M's queue, block on MVar) T2: threadDelay (insert into M's queue, block on MVar) M: process queue, performing the takeMVar for A, T1, T2 A: killThread T2 ... blocks, because T2 is now ready to run and has exceptions blocked! T1: throws TimeOut to A, which succeeds. T2: throws TimeOut to A, which is enqueued. A: catches T1 in the inner timeout, re-raises T1 to the outer timeout, completes the call. A: Now catches the pending TimeOut from T2. }}} (Examples using multiple capabilities running in parallel are easier to construct.) -- Ticket URL: http://hackage.haskell.org/trac/ghc/ticket/7719#comment:1 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#7719: System.Timeout.timeout may leak <<timeout>> exceptions -------------------------+-------------------------------------------------- Reporter: int-e | Owner: Type: bug | Status: new Priority: normal | Component: libraries/base Version: 7.7 | Keywords: Os: Linux | Architecture: x86_64 (amd64) Failure: None/Unknown | Blockedby: Blocking: | Related: -------------------------+-------------------------------------------------- Comment(by int-e): For the record, I did a quick check, implementing my own {{{threadDelay}}} without {{{_mask}}}. I still get leaked {{{TimeOut}}} exceptions, even with a single capability, but much less reliably. -- Ticket URL: http://hackage.haskell.org/trac/ghc/ticket/7719#comment:2 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#7719: System.Timeout.timeout may leak <<timeout>> exceptions -------------------------+-------------------------------------------------- Reporter: int-e | Owner: Type: bug | Status: new Priority: normal | Component: libraries/base Version: 7.7 | Keywords: Os: Linux | Architecture: x86_64 (amd64) Failure: None/Unknown | Blockedby: Blocking: | Related: -------------------------+-------------------------------------------------- Comment(by int-e): Together with nus on IRC, we found a possible solution to that latest puzzle: {{{timeout}}} creates threads using {{{forkIOWithUnmask}}} but those threads will still have exceptions blocked initially - so if the thread is never run, then a {{{killThread}}} call targeting it may still block. -- Ticket URL: http://hackage.haskell.org/trac/ghc/ticket/7719#comment:3 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#7719: System.Timeout.timeout may leak <<timeout>> exceptions -------------------------+-------------------------------------------------- Reporter: int-e | Owner: Type: bug | Status: new Priority: normal | Component: libraries/base Version: 7.7 | Keywords: Os: Linux | Architecture: x86_64 (amd64) Failure: None/Unknown | Blockedby: Blocking: | Related: -------------------------+-------------------------------------------------- Comment(by int-e): Replying to [comment:1 int-e]:
(Examples using multiple capabilities running in parallel are easier to construct.)
On second thought, no. Even with multiple capabilities, everything hinges on the fact that the {{{killThread}}} targeting the timer threads may block. Other than having exceptions masked I see no such possibility. (Blocking on black holes is a worrisome option; that is another reason (besides performance) why {{{registerTimeout}}} should '''not''' evaluate the new timeout queue.) -- Ticket URL: http://hackage.haskell.org/trac/ghc/ticket/7719#comment:4 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#7719: System.Timeout.timeout may leak <<timeout>> exceptions -------------------------+-------------------------------------------------- Reporter: int-e | Owner: Type: bug | Status: new Priority: normal | Component: libraries/base Version: 7.7 | Keywords: Os: Linux | Architecture: x86_64 (amd64) Failure: None/Unknown | Blockedby: Blocking: | Related: -------------------------+-------------------------------------------------- Comment(by int-e): Finally, I've now tried Simon M.'s suggested fix, which is simply to change {{{killThread}}} in {{{System.Timeout.timeout}}} to {{{uninterruptibleMask_ . killThread}}}, and with that change I can no longer reproduce the issue. -- Ticket URL: http://hackage.haskell.org/trac/ghc/ticket/7719#comment:5 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#7719: System.Timeout.timeout may leak <<timeout>> exceptions -------------------------+-------------------------------------------------- Reporter: int-e | Owner: Type: bug | Status: new Priority: normal | Component: libraries/base Version: 7.7 | Keywords: Os: Linux | Architecture: x86_64 (amd64) Failure: None/Unknown | Blockedby: Blocking: | Related: -------------------------+-------------------------------------------------- Comment(by int-e): (further comments on my patches above) Personally I think the bugfix is a no-brainer; the hard part was really figuring out why just masking exceptions interruptably was not enough. It still needs a test case, but I can try writing one tonight. For the faster timeout, I've asked Bas for a copy of the benchmarks he developed two years ago in connection with #4963. But I expect it to be a clear win, time-wise. -- Ticket URL: http://hackage.haskell.org/trac/ghc/ticket/7719#comment:6 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#7719: System.Timeout.timeout may leak <<timeout>> exceptions -------------------------+-------------------------------------------------- Reporter: int-e | Owner: Type: bug | Status: new Priority: normal | Component: libraries/base Version: 7.7 | Keywords: Os: Linux | Architecture: x86_64 (amd64) Failure: None/Unknown | Blockedby: Blocking: | Related: -------------------------+-------------------------------------------------- Changes (by lelf): * cc: anton.nik@… (added) -- Ticket URL: http://hackage.haskell.org/trac/ghc/ticket/7719#comment:7 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#7719: System.Timeout.timeout may leak <<timeout>> exceptions -------------------------+-------------------------------------------------- Reporter: int-e | Owner: Type: bug | Status: new Priority: normal | Component: libraries/base Version: 7.7 | Keywords: Os: Linux | Architecture: x86_64 (amd64) Failure: None/Unknown | Blockedby: Blocking: | Related: -------------------------+-------------------------------------------------- Comment(by int-e): So I have some numbers using Bas van Dijk's benchmarks (negative speedups are slowdowns): {{{ Benchmark speedup (%) NT T1 T2 T3 NT = non-threaded willTimeout -5 34 0 -1 T1 = threaded, -N1 wontTimeout -16 68 82 82 T2 = threaded, -N2 busyWillTimeout 6 98 94 95 T3 = threaded, -N3 busyWontTimeout -7 68 79 76 externalException 0 2 4 1 (Athlon II X3 445, nestedTimeouts 0 0 0 x86-64 Linux) }}} I'll attach the benchmark code but beware that compiling criterion on ghc's head currently requires patching 7 packages, mostly for the {{{Typeable}}} changes. -- Ticket URL: http://hackage.haskell.org/trac/ghc/ticket/7719#comment:8 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#7719: System.Timeout.timeout may leak <<timeout>> exceptions -------------------------+-------------------------------------------------- Reporter: int-e | Owner: Type: bug | Status: new Priority: normal | Component: libraries/base Version: 7.7 | Keywords: Os: Linux | Architecture: x86_64 (amd64) Failure: None/Unknown | Blockedby: Blocking: | Related: -------------------------+-------------------------------------------------- Changes (by basvandijk): * cc: v.dijk.bas@… (added) Comment: Bertram, I wonder how much you can speed it up further by doing what I did in my [http://hackage.haskell.org/trac/ghc/attachment/ticket/4963/faster_timeout.dp... faster_timeout darcs patch]. Namely: * Giving the !TimeoutCallback access to the !TimeoutKey: {{{ -type TimeoutCallback = IO () +type TimeoutCallback = TimeoutKey -> IO () }}} * Changing (or adding a new) Timeout exception type which uses the !TimeoutKey for identification: {{{newtype Timeout = Timeout TimeoutKey deriving Eq}}} * Use this exception instead of the existing so we can get rid of: {{{ex <- fmap Timeout newUnique}}} which should hopefully give some speedup. (I could benchmark it this weekend if you can provide me the patched libraries to get criterion to build.) -- Ticket URL: http://hackage.haskell.org/trac/ghc/ticket/7719#comment:9 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#7719: System.Timeout.timeout may leak <<timeout>> exceptions -------------------------+-------------------------------------------------- Reporter: int-e | Owner: Type: bug | Status: new Priority: normal | Component: libraries/base Version: 7.7 | Keywords: Os: Linux | Architecture: x86_64 (amd64) Failure: None/Unknown | Blockedby: Blocking: | Related: -------------------------+-------------------------------------------------- Comment(by int-e): I tried a minimal variant of the {{{TimeoutCallback}}} change, where I just added a {{{registerTimeout'}}} variant with the modified type. But I found no significant change in the measured runtimes. -- Ticket URL: http://hackage.haskell.org/trac/ghc/ticket/7719#comment:10 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#7719: System.Timeout.timeout may leak <<timeout>> exceptions -------------------------+-------------------------------------------------- Reporter: int-e | Owner: Type: bug | Status: new Priority: normal | Component: libraries/base Version: 7.7 | Keywords: Os: Linux | Architecture: x86_64 (amd64) Failure: None/Unknown | Blockedby: Blocking: | Related: -------------------------+-------------------------------------------------- Changes (by mokus): * cc: mokus@… (added) -- Ticket URL: http://hackage.haskell.org/trac/ghc/ticket/7719#comment:11 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#7719: System.Timeout.timeout may leak <<timeout>> exceptions -------------------------------+-------------------------------------------- Reporter: int-e | Owner: Type: bug | Status: new Priority: normal | Milestone: Component: libraries/base | Version: 7.7 Keywords: | Os: Linux Architecture: x86_64 (amd64) | Failure: None/Unknown Difficulty: Unknown | Testcase: Blockedby: | Blocking: Related: | -------------------------------+-------------------------------------------- Changes (by igloo): * difficulty: => Unknown Comment: Thanks for working on this. I haven't been following this issue, but is there a patch that you think should be applied now? And is there a test that it fixes? -- Ticket URL: http://hackage.haskell.org/trac/ghc/ticket/7719#comment:12 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
Thanks for working on this. I haven't been following this issue, but is
#7719: System.Timeout.timeout may leak <<timeout>> exceptions -------------------------------+-------------------------------------------- Reporter: int-e | Owner: Type: bug | Status: new Priority: normal | Milestone: Component: libraries/base | Version: 7.7 Keywords: | Os: Linux Architecture: x86_64 (amd64) | Failure: None/Unknown Difficulty: Unknown | Testcase: Blockedby: | Blocking: Related: | -------------------------------+-------------------------------------------- Comment(by int-e): Replying to [comment:12 igloo]: there a patch that you think should be applied now? And is there a test that it fixes? I've implemented a testcase now, based on the sample program above; the patch that should definitely be applied is the minimal [attachment:0001 -Fix-System.Timeout.timeout-leaking-Timeout-exception.patch] one. The second patch is about improving performance and needs a second pair of eyes. -- Ticket URL: http://hackage.haskell.org/trac/ghc/ticket/7719#comment:13 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#7719: System.Timeout.timeout may leak <<timeout>> exceptions -------------------------------+-------------------------------------------- Reporter: int-e | Owner: Type: bug | Status: patch Priority: normal | Milestone: Component: libraries/base | Version: 7.7 Keywords: | Os: Linux Architecture: x86_64 (amd64) | Failure: None/Unknown Difficulty: Unknown | Testcase: Blockedby: | Blocking: Related: | -------------------------------+-------------------------------------------- Changes (by igloo): * status: new => patch -- Ticket URL: http://hackage.haskell.org/trac/ghc/ticket/7719#comment:14 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#7719: System.Timeout.timeout may leak <<timeout>> exceptions -------------------------------+-------------------------------------------- Reporter: int-e | Owner: Type: bug | Status: patch Priority: normal | Milestone: 7.8.1 Component: libraries/base | Version: 7.7 Keywords: | Os: Linux Architecture: x86_64 (amd64) | Failure: None/Unknown Difficulty: Unknown | Testcase: Blockedby: | Blocking: Related: | -------------------------------+-------------------------------------------- Changes (by igloo): * milestone: => 7.8.1 -- Ticket URL: http://hackage.haskell.org/trac/ghc/ticket/7719#comment:15 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#7719: System.Timeout.timeout may leak <<timeout>> exceptions -------------------------------+-------------------------------------------- Reporter: int-e | Owner: Type: bug | Status: patch Priority: normal | Milestone: 7.8.1 Component: libraries/base | Version: 7.7 Keywords: | Os: Linux Architecture: x86_64 (amd64) | Failure: None/Unknown Difficulty: Unknown | Testcase: Blockedby: | Blocking: Related: | -------------------------------+-------------------------------------------- Changes (by int-e): * cc: int-e (added) -- Ticket URL: http://hackage.haskell.org/trac/ghc/ticket/7719#comment:16 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#7719: System.Timeout.timeout may leak <<timeout>> exceptions -------------------------------+-------------------------------------------- Reporter: int-e | Owner: Type: bug | Status: patch Priority: normal | Milestone: 7.8.1 Component: libraries/base | Version: 7.7 Keywords: | Os: Linux Architecture: x86_64 (amd64) | Failure: None/Unknown Difficulty: Unknown | Testcase: Blockedby: | Blocking: Related: | -------------------------------+-------------------------------------------- Comment(by igloo): 0001-Fix-System.Timeout.timeout-leaking-Timeout-exception.patch and 0001 -add-test-for-7719.patch applied. -- Ticket URL: http://hackage.haskell.org/trac/ghc/ticket/7719#comment:17 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#7719: System.Timeout.timeout may leak <<timeout>> exceptions
-----------------------------+----------------------------------------------
Reporter: int-e | Owner:
Type: bug | Status: closed
Priority: normal | Milestone: 7.8.1
Component: libraries/base | Version: 7.7
Resolution: fixed | Keywords:
Os: Linux | Architecture: x86_64 (amd64)
Failure: None/Unknown | Difficulty: Unknown
Testcase: | Blockedby:
Blocking: | Related:
-----------------------------+----------------------------------------------
Changes (by igloo):
* status: patch => closed
* resolution: => fixed
Comment:
Optimisation also applied:
{{{
commit 31153f1e06ad05002d1a199d9c45a36fcbbaf033
Author: Bertram Felgenhauer
participants (1)
-
GHC