Beware of Thread#kill or your ActiveRecord transactions are in danger of being partially committed

Firstly, before you all say you should never use Thread#kill and link to Headius post, this is not the same issue which he explains, but it is somewhat related.

Secondly, before you ask why are you using Thread#kill anyway, or why should we worry about anyone who is. There is one scenario where Threads being #killed is quite likely and there are probably a bunch of people who run into it without knowing. When a ruby process terminates all Threads other than the main one are #killed (the main Thread gets an exception raised). So if you had a ruby process with a bunch of threads and this process received a TERM signal, all those Threads are going to get #killed unless you specifically handle the TERM signal and instruct/wait for all threads to finish. That is of course what you should do but I am guessing there are plenty of people who don’t.

ActiveRecord transactions and Thread#kill

First let me say your ActiveRecord transactions could already be partially committed if you use Thread#kill or Thread#raise due to the reasons Headius’ explains. But the chance of this happening due to those reasons is lower than due to this new one. Here’s the ActiveRecord transaction code. I’ve posted the Rails 2.2 version because it’s much simpler and less code than the current 3.0 source. But 3.0 is still vulnerable to the same issue:

      def transaction(start_db_transaction = true)
        transaction_open = false
        begin
          if block_given?
            if start_db_transaction
              begin_db_transaction
              transaction_open = true
            end
            yield
          end
        rescue Exception => database_transaction_rollback
          if transaction_open
            transaction_open = false
            rollback_db_transaction
          end
          raise unless database_transaction_rollback.is_a? ActiveRecord::Rollback
        end
      ensure
        if transaction_open
          begin
            commit_db_transaction
          rescue Exception => database_transaction_rollback
            rollback_db_transaction
            raise
          end
        end
      end

Now imagine you’re using it like this

ActiveRecord::Base.transaction do
  user_1.update_attribute :something, 1
  ...
  user_n.update_attribute :something, n
end

If Thread#kill is called anywhere inside of that block the rest of the updates will not occur but the transaction will be committed.

How does it happen?

The reason this happens and why it’s a little unexpected is because Thread#kill will bypass all code _including_ rescue blocks and jump immediately to ensure blocks. Many people code with the assumption if an ensure block is hit without a rescue block being hit the code must have finished executing. And this is the assumption made in the ActiveRecord transaction code. Imagine we move from the middle of the block passed to #transaction (where Thread#kill was called) down to the start of the ensure block. transaction_open will evaluate to true (which it would not have were an exception raised since the rescue block sets transaction_open to false) and commit_db_transaction will be called.

In contrast, for this to occur due to the Headius’ issue first an exception would have to be raised inside the transaction block and _then_ Thread#raise or Thread#kill would have to be called inside the rescue block before transaction_open is set to false, a much smaller area of attack.

Can this be fixed?

Yes and no. Setting a local variable to true after the yield statement is a simple way to determine if the block passed to yield ran to completion:

begin
  yield
  completed = true
ensure
  puts "entire block ran!"  if completed
end

This assumes the code in the block doesn’t use certain flow control statements. Those statements are return, break, next, retry, redo and throw. If one of these statements is used to break out of the block prematurely it will effectively act just like Thread.kill. The interpreter will immediately jump to the ensure block bypassing everything else including the local variable being set. Which means if you relied on the local variable being set to commit the transaction then any transaction block with one of those statements would always be rolled back.

In MRI 1.8 and Rubinius Thread.current.status will be set to ‘aborting’ if the current thread is in the state of being killed. So we can detect if we are in the ensure block because of a Thread#kill rather than one of the flow control statements.

begin
  yield
ensure
  puts "enitre block ran!"  if Thread.current.status != 'aborting'
end

The problem is this doesn’t work at all in 1.9 and isn’t consistent in JRuby. If all interpreters behaved as 1.8 does then fixing this issue would be trivial and have no downsides. Knowing whether you are in an ensure block prematurely because the programmer intended so via flow control statements versus due to an aborting thread seems fairly important. So I believe that 1.9 and JRuby should copy this behavior or at least provide some other similar mechanism.

The Patch

Here’s a patch which fixes the issue for MRI 1.8 and Rubinius:
https://github.com/coderrr/rails/commit/7f8ac269578e847329c7cfb2010961d3e447fd19

Thanks to Marcos Toledo for brainstorming the issue with me and helping me proofread