coderrr

May 3, 2011

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

Filed under: concurrency, patch, rails — Tags: , , — coderrr @ 3:25 pm

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

4 Comments »

  1. Is there a link to a pull request or github issue so this can get fixed?

    Comment by Eric Hodel — May 4, 2011 @ 12:10 am

  2. The real problem is that the code inside the ensure block goes against the semantic of transactions. If anything it should contain a rollback not a commit, the rationale being that commit is the last operation inside regular code; if you exit normal flow for any reason and the transaction is still open you should make a rollback because at that point you can only be sure that something went wrong.
    I would do it like this

    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
          commit_db_transaction
          transaction_open = false
        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
        rollback_db_transaction
      end
    end
    

    Comment by Paolo Angelini — May 6, 2011 @ 7:11 pm

    • You’re assuming exiting normal flow includes using ” return, break, next, retry, redo and throw ” inside your transaction block. Ruby encourages the use of these as normal flow control constructs. Returning from a method early due to a guard is extremely common as an alternative to nesting if/else statements.

      This brings up an interesting issue with Ruby. Ensure is not only for making sure some code runs whether or not an exception is raised, but it is necessary to use an ensure to make sure code runs in case a block is exited early due to one of these flow control statements.

      If Thread#kill didn’t existed there would be no problem. Because you could assume if you entered an ensure block without previously entering a rescue block then you must be there due to a flow control statement and the block finished executing as the programmer intended. Thread#kill introduces the ambiguity, you might be there because the executed as intended, or because the thread was #killed. If Thread#kill raised an exception like Thread#raise rather than immediately jumping you to the ensure block none of this would be an issue…

      Comment by coderrr — May 6, 2011 @ 8:32 pm


RSS feed for comments on this post. TrackBack URI

Leave a Reply

Please log in using one of these methods to post your comment:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

The Silver is the New Black Theme. Create a free website or blog at WordPress.com.

Follow

Get every new post delivered to your Inbox.

Join 28 other followers

%d bloggers like this: