Stuck transactions

[jeffstyr]

Okay this was a tricky one to track down. I noticed a case of having open_transactions > 0 even though there were no clients connected. (When this happened there had been a message in the queue larger than GrabbyHands was configured to accept, so it would start to read it, snap the connection, and repeat.) The key is having an uncommitted transaction and a waiter, and having both connections snap "simultaneously" (e.g. both coming from the same process, and that process gets killed). Of course it's a race so it doesn't happen every time.

Take a look at this stacktrace: https://gist.github.com/dde96d230fb0c25defe3

It seems that what's happening is this: A connection with a pending transaction closes, and NettyHandler.handleUpstream(ChannelStateEvent case) calls finish() to cancel the outstanding transactions in that connection. Fine so far. If there is a waiter, then the unremove triggers the waiter which opens a new transaction and fulfills its Future, which causes its connection to write out the response. BUT, if that second connection is already closed, this causes a ClosedChannelException, when ends up in NettyHandler.handleUpstream(ExceptionEvent case), but if the ChannelStateEvent for that channel's close already happened, then nothing cleans up the newly-opened transaction, so it's stuck until you bounce kestrel. Phew. So the fix for this is to just to call finish() when NettyHandler.handleUpstream() gets handed a ClosedChannelException. That change is in jeffstyr/kestrel@97e3fdbc29fa5cc362bdffe77dc35cf65e22b7c6

So that was all great, until I got a deadlock. See https://gist.github.com/7560313bf9903d328c16. The problem now is that finish() calls KestrelHandler.pendingTransactions.cancelAll(), which holds a lock while it does the unremoves, which locks the PersistentQueue, which calls the trigger/write/exception/finish, which then blocks trying to lock another KestrelHandler.pendingTransactions, for which another thread is already holding the lock and blocked trying to acquire the lock on the PersistentQueue for its own finish. Phew again. So the fix for this is to do the actual removes outside of the lock in KestrelHandler.pendingTransactions.cancelAll. That change is in jeffstyr/kestrel@891d1a264c96a35023438fd014fd1ebb139a1879

But it occurred to me that this chain of fulfilling futures in closed connections could continue to recurse deeper and deeper (in theory, continuing on through all the reamining waiters), which could blow the stack. (Haven't seen this actually happen though.) So I think it's safer to do the actual cancellAll-originated unremoves asynchronously via an ExecutorService; this puts a constant bound on the stack depth, and also (correspondingly) leaves less opportunity for something else to be holding onto a lock while all this is happening. That change is in jeffstyr/kestrel@b317ea5dfa908024c93a724e2fe941c8e45e5ec5 . It plumbs through the ExectorService we are already creating in Kestrel.scala.

That third change is a bigger change, so I'd appreciate feedback on that one especially, of course.

Side comment: It would be nice to proactively cancel these Futures when their corresponding connection breaks, but you can't really get to them to do it, I think.

All of these fixes are on this branch in my fork: https://github.com/jeffstyr/kestrel/tree/handle-connection-exception. I can send a pull request if the changes sound good to you.

[robey]

wow, thanks for digging into this!

your analysis sounds right to me. i just added options to "many_clients" to allow the clients to be transactional and to kill off N% of them on each round. i only had to try that once, with 10% kill rate, to get the test to fail to complete (on master). open transactions just got lost. merging in your first 2 fixes makes the test complete.

i'm going to merge those into master.

[robey]

the stack-depth problem is pretty interesting. we have a branch of kestrel (release_3_0) that uses finagle instead of netty directly -- since finagle is netty-based, it really just lets us remove some glue code. and finagle has occasionally had this issue with its recursive callbacks.

havoc wrote a nice blog post about this problem:

http://blog.ometer.com/2011/07/24/callbacks-synchronous-and-asynchronous/

i think the right answer is to modify the future library (in util-core) to allow configuring a "completion" thread-pool (really, an executor) where completed futures are posted. that way, fulfulling a future would never start another nested call.

[jeffstyr]

Cool sounds good.

I feel like I've been running into different version of the general problem recently too. Handling it in the core of the future library makes sense, and always calling them asynchronously (as that blog post advises) solves the opposite-of-deadlock problem in that running them in the caller's thread lets them blow through recursive/intrinsic locks held by that thread, and it might be better for them to not. (That is, you might be holding a lock which is supposed to prevent some data structure from changing, and could be taken totally unawares that some action you are taking is triggering a Future which when modifies that data structure. Done async, the Future would be blocked from touching it while you hold the lock, as you'd want.)

It's somewhat interesting that while this would solve the stack depth problem, in the current case the Future-callback-execution is sort of only "half" the stack, and so lock-holding is still more finicky than if the async part started at the unremove call. But there's not a general way to make that happen, and for a specific-to-this-case solution it would probably be better to have a handler explicitly know about its Futures, so that it can proactively cancel them. (That is, it's sort of icky that when a connection breaks, the corresponding Futures are only cleaned out as side effects of destined-to-be-undone remove()+write() sequences. But, directly keeping track of them in the handler is much less slick than how things work now.)

As a side note, I was surprised I couldn't find anywhere the Scala-ized version of an Executor (taking a lambda rather than a Runnable). Trivial to write, but I didn't see one around anywhere already.

Thanks much!

[robey]

i think the problem stated in the bug is now fixed.