[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
#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]:
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?
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 <int-e@gmx.de> Date: Thu Mar 7 02:19:28 2013 +0100 Implement faster System.Timeout.timeout for the threaded RTS. The basic idea is to use the Timer Manager rather than spawning an auxilliary thread to handle timeouts; this was proposed by Bas van Dijk two years ago (#4963), and recently by Herbert Valerio. The key idea to make this work is to spawn an auxilliary thread for the delivery of the Timeout exception. This idea as well as most of the code is due to Akio Takano. }}} -- Ticket URL: <http://hackage.haskell.org/trac/ghc/ticket/7719#comment:18> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
participants (1)
-
GHC