Notify method not threadsafe

[jasonayre]

https://github.com/SquareSquash/ruby/blob/master/lib/squash/ruby.rb#L65

The notify method is not threadsafe, not sure I can instruct you of an easy way to reproduce, so I'll detail the situation.

Noticed it when we upgraded our rails app to jruby and trinidad, over from unicorn. I have an angular app which is hitting 3-4 controller endpoints when the page loads, so 3 new threads are being spawned. Initially in development mode, when you hit the 3 controllers endpoints for the first time, 2 of them generally throw errors (has since been fixed, but this is happening in multiple places in our platform so I tracked down why squash was breaking, as we were unable to consistently reproduce until now) -- The result of the first page load is, the two threads which are receiving the errors, both seem to be calling Squash::Ruby.notify at the same time. The result of this was strange, but basically, the threads that were throwing the errors in parallel were eating up all the memory in the JVM until it threw a Tomcat JVM ran out of memory error, about 1-2 minutes later.

A symptom of the thread safety issue, which was causing the OOM error, can be seen here:

https://github.com/SquareSquash/ruby/blob/master/lib/squash/ruby.rb#L585

When I traced it down to the json call, removed it from the rescue block and outputted, I got a ActiveSupport::JSON::Encoding recursion error, something about trying to call to_json on object when it had already been called. If I moved the filtered.to_json out of the rescue block, it broke and actually trasmitted the error, without hanging up and running the app out of memory. Also saw while outputting the error hash that gets created, that the contents of the json were being serialized 3 times, i.e. \ escaping the quotes.

The thread safety issue was confirmed when I wrapped the notify method with Thread.exclusive, which fixed the issue alltogether. I.E.

    def self.notify(exception, user_data={})
      Thread.exclusive do
        raise "The :api_key configuration is required" unless configuration(:api_key)
        raise "The :api_host configuration is required" unless configuration(:api_host)
        raise "The :environment configuration is required" unless configuration(:environment)

        begin
          blob = self.generate_exception(exception, user_data)
          return false if blob.nil?

          self.transmit_exception(blob)
          return true
        rescue Object => nested_error
          raise if configuration(:disable_failsafe)
          failsafe_handler exception, nested_error
          :failsafe # a perfect example of http://thedailywtf.com/Articles/What_Is_Truth_0x3f_.aspx
        end
      end
    end

However this probably isn't a great fix as the threads will be waiting on lock to run.

Let me know if you need anymore info.