{"action":"created","issue":{"url":"https://api.github.com/repos/slick/slick/issues/1461","repository_url":"https://api.github.com/repos/slick/slick","labels_url":"https://api.github.com/repos/slick/slick/issues/1461/labels{/name}","comments_url":"https://api.github.com/repos/slick/slick/issues/1461/comments","events_url":"https://api.github.com/repos/slick/slick/issues/1461/events","html_url":"https://github.com/slick/slick/pull/1461","id":140905020,"number":1461,"title":"fixes issue 1274: Slick deadlock","user":{"login":"kwark","id":629786,"avatar_url":"https://avatars.githubusercontent.com/u/629786?v=3","gravatar_id":"","url":"https://api.github.com/users/kwark","html_url":"https://github.com/kwark","followers_url":"https://api.github.com/users/kwark/followers","following_url":"https://api.github.com/users/kwark/following{/other_user}","gists_url":"https://api.github.com/users/kwark/gists{/gist_id}","starred_url":"https://api.github.com/users/kwark/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/kwark/subscriptions","organizations_url":"https://api.github.com/users/kwark/orgs","repos_url":"https://api.github.com/users/kwark/repos","events_url":"https://api.github.com/users/kwark/events{/privacy}","received_events_url":"https://api.github.com/users/kwark/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":4,"created_at":"2016-03-15T08:24:07Z","updated_at":"2016-03-24T15:01:57Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/slick/slick/pulls/1461","html_url":"https://github.com/slick/slick/pull/1461","diff_url":"https://github.com/slick/slick/pull/1461.diff","patch_url":"https://github.com/slick/slick/pull/1461.patch"},"body":"This should fix issue https://github.com/slick/slick/issues/1274\r\n\r\nSolution is as described in https://groups.google.com/forum/?utm_medium=email&utm_source=footer#!msg/scalaquery/5MCUnwaJ7U0/-wTi7lVoBQAJ\r\n\r\nIntroduction of priority levels:\r\n- HighPriority => a DBIO which already has a connection associated (due to running in a transaction or with pinned session)\r\n- MediumPriority => the old highPrio = true case: a continuation of another DBIO action, it should always be able to be enqueued\r\n- LowPriority => any other DBIO, if the queue is full it will not be enqueued\r\n\r\nThe queue backing Slick's ThreadPoolExecutor now consists internally of two backing queues:\r\n- A HighPriority queue which contains only the HighPriority runnables\r\n- The old queue containing the other priority levels.\r\n\r\nHighPriority items are always taken first, until this queue is exhausted and only then Low- or MediumPriority items are considered.\r\n\r\nWe also do connection counting in the HikariCPJdbcDataSource:\r\n- When there are no more connections in the pool, we prevent low/medium priority items from being processed from the queue. Only HighPriority items are able to make progress, because they already have a connection.\r\n\r\nAdded a SlickDeadlock test: while this was failing with previous versions, it now is able to run successfully.\r\n\r\n**I'm explicitly soliciting feedback, because this is a possibly high impact change**"},"comment":{"url":"https://api.github.com/repos/slick/slick/issues/comments/200875073","html_url":"https://github.com/slick/slick/pull/1461#issuecomment-200875073","issue_url":"https://api.github.com/repos/slick/slick/issues/1461","id":200875073,"user":{"login":"szeiger","id":54262,"avatar_url":"https://avatars.githubusercontent.com/u/54262?v=3","gravatar_id":"","url":"https://api.github.com/users/szeiger","html_url":"https://github.com/szeiger","followers_url":"https://api.github.com/users/szeiger/followers","following_url":"https://api.github.com/users/szeiger/following{/other_user}","gists_url":"https://api.github.com/users/szeiger/gists{/gist_id}","starred_url":"https://api.github.com/users/szeiger/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/szeiger/subscriptions","organizations_url":"https://api.github.com/users/szeiger/orgs","repos_url":"https://api.github.com/users/szeiger/repos","events_url":"https://api.github.com/users/szeiger/events{/privacy}","received_events_url":"https://api.github.com/users/szeiger/received_events","type":"User","site_admin":false},"created_at":"2016-03-24T15:01:56Z","updated_at":"2016-03-24T15:01:56Z","body":"After thinking about this some more and rereading my analysis from the mailing list discussion, I think that counting actual Connections may not be a good idea after all. Intercepting anything in the Connection or at the point the Connection is made, is too late. I have not been able to come up with a situation where this solution will still lead to a deadlock (but I wouldn't rule it out entirely). It could, however, impact performance negatively.\r\n\r\nTake a scenario similar to the deadlock case. Note that the `map` step for the first action still takes 100ms but the others now take 200ms:\r\n\r\n```\r\n  val tasks = 1 to 50 map { c =>\r\n    val a = sql\"select 1\".as[Int].head.map { i => Thread.sleep(if(c == 1) 100 else 200); i }\r\n    dc.db.run(a.transactionally)\r\n  }\r\n  Await.result(Future.sequence(tasks), Duration(30, TimeUnit.SECONDS))\r\n```\r\n\r\nThis time we assume 10 threads, 10 connections and a large queue. Now you can get the following chain of events:\r\n\r\n1. The first 10 SynchronousDatabaseActions are started. They pin the session, acquire a Connection (thus incrementing the counter) and perform the query.\r\n\r\n2. When any of those actions is finished, the pool threads stall because there are no free Connections, so the queue does not deliver the next actions (which are all of low priority).\r\n\r\n3. After 100ms, the first action's mapping step is done, so another SynchronousDatabaseAction is enqueued with high priority (because it already has a Connection).\r\n\r\n4. This action is picked up by a pool thread. It commits the transaction and closes the Connection.\r\n\r\n5. At this point the queue starts returning low-priority actions again (because there is a free Connection available and the high-priority continuations of the other 9 mapping steps are not ready yet for another 100ms), so all 10 threads could get a new low-priority action before one of them gets to the point where it actually needs a Connection (and thus decrements the counter of free Connections).\r\n\r\nThe difference to the current situation is that one thread will eventually get a Connection, so progress is guaranteed. There is no more risk of deadlock. But the other 9 threads block on `getConnection` and eventually fail after 1 second while the high-priority continuations may get funneled through the single thread that is still able to make progress.\r\n\r\nWith this in mind, we shouldn't really be counting *Connections*, we should count *SynchronousDatabaseActions*, and do it atomically. While it is possible to write a SynchronousDatabaseAction that never tries to acquire a Connection, it is an atypical case that we do not have to optimize for (but we do have to take it into account for correctness). Let's assume that every SynchronousDatabaseAction either has an open Connection or will have to open one. `take` could increment the \"open\" counter before returning a non-high-priority action. This would prevent the race condition in (5) above. When a SynchronousDatabaseAction finishes, you have to detect the case when it closed a Connection and then decrement the \"open\" counter (and possibly signal threads that are blocked on `take`). This probably needs to be baked into the interpreter's state machine. You cannot rely on `Connection.close` because you do not know if a new SynchronousDatabaseAction ever actually opened a Connection."}}