{"action":"created","issue":{"url":"https://api.github.com/repos/ztellman/manifold/issues/148","repository_url":"https://api.github.com/repos/ztellman/manifold","labels_url":"https://api.github.com/repos/ztellman/manifold/issues/148/labels{/name}","comments_url":"https://api.github.com/repos/ztellman/manifold/issues/148/comments","events_url":"https://api.github.com/repos/ztellman/manifold/issues/148/events","html_url":"https://github.com/ztellman/manifold/issues/148","id":272494055,"node_id":"MDU6SXNzdWUyNzI0OTQwNTU=","number":148,"title":"Deadlock when using manifold.time scheduling in application code","user":{"login":"gsnewmark","id":669671,"node_id":"MDQ6VXNlcjY2OTY3MQ==","avatar_url":"https://avatars3.githubusercontent.com/u/669671?v=4","gravatar_id":"","url":"https://api.github.com/users/gsnewmark","html_url":"https://github.com/gsnewmark","followers_url":"https://api.github.com/users/gsnewmark/followers","following_url":"https://api.github.com/users/gsnewmark/following{/other_user}","gists_url":"https://api.github.com/users/gsnewmark/gists{/gist_id}","starred_url":"https://api.github.com/users/gsnewmark/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/gsnewmark/subscriptions","organizations_url":"https://api.github.com/users/gsnewmark/orgs","repos_url":"https://api.github.com/users/gsnewmark/repos","events_url":"https://api.github.com/users/gsnewmark/events{/privacy}","received_events_url":"https://api.github.com/users/gsnewmark/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":2,"created_at":"2017-11-09T09:29:49Z","updated_at":"2018-11-27T16:57:10Z","closed_at":null,"author_association":"NONE","body":"Right now default `manifold.time` scheduler is based on [single-thread executor](https://github.com/ztellman/manifold/blob/c6217cbe35fda82f120ee6fcef3a48bfc9ca7202/src/manifold/time.clj#L207-L212) which is used both internally in Manifold itself (e. g., for [deferred timeouts](https://github.com/ztellman/manifold/blob/c6217cbe35fda82f120ee6fcef3a48bfc9ca7202/src/manifold/deferred.clj#L1158-L1161)) and in public scheduling API (namely `manifold.time/in` & `manifold.time/every`). This can lead to nasty deadlock bugs when seemingly unconnected parts of application stop working due to blocking in scheduler. For example, following code will never finish because inner timeout will wait for scheduler thread blocked by the scheduled function itself:\r\n\r\n```\r\nuser=> (require '[manifold.deferred :as d])\r\nnil\r\nuser=> (require '[manifold.time :as t])\r\nnil\r\nuser=> @(t/in 1000 (fn [] @(d/timeout (d/deferred) 100 ::inner-timeout)))\r\n\r\n```\r\n\r\nThis example is artificial, but we actually had similar situation in real code recently (just with more indirection involved). \r\n\r\n`manifold.time` allows passing of custom clocks/executors to scheduling function ([`with-clock`](https://github.com/ztellman/manifold/blob/c6217cbe35fda82f120ee6fcef3a48bfc9ca7202/src/manifold/time.clj#L218-L223)), so that's not really a major issue. But still should documentation be updated to clearly reflect that using `manifold.time` in application code as is can cause issues with other parts of Manifold? Alternatively, should two separate clocks be created by default - one for Manifold's own scheduling needs and one for all other `manifold.time` users (on the first glance I can't see problems with this approach, but maybe I'm missing something)?"},"comment":{"url":"https://api.github.com/repos/ztellman/manifold/issues/comments/442134669","html_url":"https://github.com/ztellman/manifold/issues/148#issuecomment-442134669","issue_url":"https://api.github.com/repos/ztellman/manifold/issues/148","id":442134669,"node_id":"MDEyOklzc3VlQ29tbWVudDQ0MjEzNDY2OQ==","user":{"login":"kachayev","id":485647,"node_id":"MDQ6VXNlcjQ4NTY0Nw==","avatar_url":"https://avatars1.githubusercontent.com/u/485647?v=4","gravatar_id":"","url":"https://api.github.com/users/kachayev","html_url":"https://github.com/kachayev","followers_url":"https://api.github.com/users/kachayev/followers","following_url":"https://api.github.com/users/kachayev/following{/other_user}","gists_url":"https://api.github.com/users/kachayev/gists{/gist_id}","starred_url":"https://api.github.com/users/kachayev/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/kachayev/subscriptions","organizations_url":"https://api.github.com/users/kachayev/orgs","repos_url":"https://api.github.com/users/kachayev/repos","events_url":"https://api.github.com/users/kachayev/events{/privacy}","received_events_url":"https://api.github.com/users/kachayev/received_events","type":"User","site_admin":false},"created_at":"2018-11-27T16:57:10Z","updated_at":"2018-11-27T16:57:10Z","author_association":"CONTRIBUTOR","body":"@joeltio I've tried a few different versions of your code but it seems I cannot reproduce the issue with it. It works fine from the first glance.\r\n\r\n@gsnewmark Regarding your example. I know where this issue comes from and I do understand how hard to debug something like that in a large codebase... but semantically, if you block a thread from whatever seems to be an \"async\" context (whatever that means technically), you always have problems. That's a curse of implementing async flows on top of JVM runtime, you should always keep in mind that there's a thread pool somewhere underneath the API and this thread pool might end up in the situation where all threads are blocked. `core.async` is a great example of how leaky this abstraction and how fragile the idea to write the program w/o understanding of the underlying threads mechanics. \r\n\r\nWith that being said I think there's no ideal solution here, what we can do:\r\n\r\n1. Mention in the documentation that blocking operations inside `chain`/`catch`/`in`/`every` etc are very risky and you should think twice what thread pool backs each callback. Not specifically for `manifold.time` but for all callbacks.\r\n\r\n2. Introduce a new API that doesn't require `time/*` callbacks to run on a scheduling thread. Just to make sure that schedule thread is doing only one thing: deliver values at some point in time. And the actual execution of callbacks (either provided or added as listeners using `chain`) happens somewhere else. I.e. `time/every-on` that requires an executor to be provided, an option to create a deferred that will be realized to a specific value after delay so you can use it with `alt` (think `core.async/timeout` channels).\r\n\r\n@ztellman what do you think? "}}
{"actor":{"display_login":"kachayev"}}