{"action":"created","issue":{"url":"https://api.github.com/repos/scalaz/scalaz-stream/issues/326","labels_url":"https://api.github.com/repos/scalaz/scalaz-stream/issues/326/labels{/name}","comments_url":"https://api.github.com/repos/scalaz/scalaz-stream/issues/326/comments","events_url":"https://api.github.com/repos/scalaz/scalaz-stream/issues/326/events","html_url":"https://github.com/scalaz/scalaz-stream/pull/326","id":59998843,"number":326,"title":"Cleaner runAsync without redundant cleanups","user":{"login":"djspiewak","id":752,"avatar_url":"https://avatars.githubusercontent.com/u/752?v=3","gravatar_id":"","url":"https://api.github.com/users/djspiewak","html_url":"https://github.com/djspiewak","followers_url":"https://api.github.com/users/djspiewak/followers","following_url":"https://api.github.com/users/djspiewak/following{/other_user}","gists_url":"https://api.github.com/users/djspiewak/gists{/gist_id}","starred_url":"https://api.github.com/users/djspiewak/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/djspiewak/subscriptions","organizations_url":"https://api.github.com/users/djspiewak/orgs","repos_url":"https://api.github.com/users/djspiewak/repos","events_url":"https://api.github.com/users/djspiewak/events{/privacy}","received_events_url":"https://api.github.com/users/djspiewak/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/scalaz/scalaz-stream/labels/design/refactoring","name":"design/refactoring","color":"fbca04"}],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":32,"created_at":"2015-03-05T19:02:17Z","updated_at":"2015-03-10T14:24:03Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/scalaz/scalaz-stream/pulls/326","html_url":"https://github.com/scalaz/scalaz-stream/pull/326","diff_url":"https://github.com/scalaz/scalaz-stream/pull/326.diff","patch_url":"https://github.com/scalaz/scalaz-stream/pull/326.patch"},"body":"There's really no reason (that I can think of) for `runAsync` to be implemented using raw `Actor`.  This reimplements it using `Task`, also fixing a bug regarding duplicated `onComplete` evaluation, cleaning up the signaling, making the interrupt mechanism more responsive, and also opens the door to a more reasonable type signature down the road (propagating the `Task` context into `njoin` and `wye`).\r\n\r\nThe first of the two really notable changes here are that we are now using `Task`'s `runAsyncInterruptibly`, which gives us the ability to kill a `Task` (within limits).  This is a strict improvement over the old `Process#runAsync`, which would literally leak threads whenever a `Task` continued running for a long time after the interrupt.  This happens, for example, if we do a `Process.await(p.run) { ... }`, where `p` is some long-running process, running `Process#runAsync` on that result would leak a LOT of computational resources.  No more!\r\n\r\nThe second change is that we are no longer invoking finalizers multiple times.  This is a really subtle issue, but you can see it here:\r\n\r\n```scala\r\n              case Interrupt(cause) if completed.isEmpty =>\r\n                completed = Some(cause)\r\n                Try(cleanup(cause)).run.runAsync(_.fold(\r\n                  rsn0 =>  cb(left(Error(rsn0).causedBy(cause)))\r\n                  , _ => cb(left(cause))\r\n                ))\r\n```\r\n\r\n…and here!\r\n\r\n```scala\r\n              case AwaitDone(r, awt, _) =>\r\n                Try(awt.rcv(EarlyCause(r)).run)\r\n                .kill\r\n                .run.runAsync(_ => ())\r\n```\r\n\r\nCombined with this:\r\n\r\n```scala\r\n              case Step(awt@Await(req, rcv), cont) =>\r\n                req.runAsync(r => a ! AwaitDone(r, awt, cont))\r\n                right((c:EarlyCause) => rcv(left(c)).run +: cont)\r\n```\r\n\r\nSo with the *old* `runAsync`, when we interrupt a process, we immediately run its cleanup, but we don't terminate the task!  Thus, the task will eventually complete… and then run the whole process again.  Hurray!\r\n\r\nThis PR fixes several of the issues that we (@alissapajer and I) have seen in concurrent code, including several out the outstanding \"`wye` issues\"."},"comment":{"url":"https://api.github.com/repos/scalaz/scalaz-stream/issues/comments/78063228","html_url":"https://github.com/scalaz/scalaz-stream/pull/326#issuecomment-78063228","issue_url":"https://api.github.com/repos/scalaz/scalaz-stream/issues/326","id":78063228,"user":{"login":"edwinchoi","id":773193,"avatar_url":"https://avatars.githubusercontent.com/u/773193?v=3","gravatar_id":"","url":"https://api.github.com/users/edwinchoi","html_url":"https://github.com/edwinchoi","followers_url":"https://api.github.com/users/edwinchoi/followers","following_url":"https://api.github.com/users/edwinchoi/following{/other_user}","gists_url":"https://api.github.com/users/edwinchoi/gists{/gist_id}","starred_url":"https://api.github.com/users/edwinchoi/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/edwinchoi/subscriptions","organizations_url":"https://api.github.com/users/edwinchoi/orgs","repos_url":"https://api.github.com/users/edwinchoi/repos","events_url":"https://api.github.com/users/edwinchoi/events{/privacy}","received_events_url":"https://api.github.com/users/edwinchoi/received_events","type":"User","site_admin":false},"created_at":"2015-03-10T14:24:03Z","updated_at":"2015-03-10T14:24:03Z","body":"This will not work in this case:\r\n```scala\r\nval a : lock\r\nval b:lock\r\n\r\nawait(a.acquire) { _=>\r\n  await(b.acquire) { _=>\r\n    doSomethingDangerous().onComplete(\r\n      eval_(b.release))\r\n  }.onComplete(eval_(a.release))\r\n}\r\n```\r\n\r\nWithout interrupt semantics, `doSomethingDangerous` runs with exclusive access to the resources protected by `a` and `b`.\r\nWith interrupt semantics this is no longer the case, since interrupt could cause `a` to be released while `doSomethingDangerous` is still running... Which would allow another thread to acquire the lock and proceed as if they had exclusive access.\r\n\r\nThe lack of support for sequential reasoning makes it much more difficult to verify overall correctness (can't simply look at sequential execution). And the burden of reasoning about overlapping computations is placed on the user.. These are difficult pills to swallow and makes devising correct solutions very difficult.\r\n\r\nNote: the example above will never work with interrupt semantics as they've been described. In fact, there is no way to safely share resources beyond a single step with interrupt in the picture.\r\nThis was one of the reasons I suggested `onPreempt`. `onPreempt` would provide external threads with the means to tell the process to halt without having to call the finalizers directly, which breaks isolation. `onPreempt` would not be a finalizer, the process would still have to wait for await to complete before halting.\r\n\r\nThis may not be the cleanest approach, but it is amenable to sequential reasoning.\r\nAlso, having separate `onPreempt` and `onComplete` combinators (vs EarlyCause as @djspiewak had suggested earlier) is that the execution/ownership boundaries are clear and explicit. Which in my opinion is a plus."}}