Upgrading Discourse to Rails 6

Hi Team,

Rails 6.0.0 was released 25 days ago so I think it is a time to update Discourse :slight_smile: There were a few steps I needed to do to make that work.

  1. Fix broken specs
  • Add empty method trigger_transactional_callbacks? to lib/mini_sql_multisite_connection.rb
  • UrlHelper is loading by default ActionView::Helpers::UrlHelper instead of lib/UrlHelper. I solved it by adding :: in front, however, what do you think about changing the name of that class?
  • In Rails 5.2.3 MigrationContext accepted one argument, in 6.0.0 additional schema is required
  1. Fix depracated methods
  • Update_attributes! replaced with update!
  • Content_type will contain charset, media type should be used instead - rails/upgrading_ruby_on_rails.md at master · rails/rails · GitHub
  • Fix warning already initialized constant (TRADITIONAL_ESCAPED_CHAR, RFC_5987_ESCAPED_CHAR). Before dropped them I confirmed that values in ActionPack are the same
  1. Use classic autoloader as the first step before Zeitwerk
  2. Fixed migrations on Rails 6.0.0 - newest rails is not allowing mistake from old migration (you can’t define an already defined column ‘integer’)

I made smoke tests that Discourse work as expected. In addition, I run performance tests to ensure there is no regression (I used default 500 iterations).

Test Rails 5.2.3 Rails 6.0.0 Percent
categories-50 27 24 88.89%
categories-75 31 26 83.87%
categories-90 36 37 102.78%
categories-99 52 50 96.15%
home-50 27 26 96.30%
home-75 30 28 93.33%
home-90 39 38 97.44%
home-99 53 55 103.77%
topic-50 35 27 77.14%
topic-75 36 29 80.56%
topic-90 37 39 105.41%
topic-99 56 50 89.29%
categories_admin-50 47 47 100.00%
categories_admin-75 54 59 109.26%
categories_admin-90 64 66 103.13%
categories_admin-99 132 116 87.88%
home_admin-50 47 46 97.87%
home_admin-75 51 56 109.80%
home_admin-90 63 64 101.59%
home_admin-99 110 97 88.18%
topic_admin-50 50 49 98.00%
topic_admin-75 58 59 101.72%
topic_admin-90 65 67 103.08%
topic_admin-99 113 86 76.11%
load_rails 2593 2618 100.96%
rss_kb 318800 287332 90.13%
pss_kb 306913 275378 89.73%
Average 89.31%

I will create a pull request will all the changes mentioned above. Please let me know if you would like me to adjust anything or made additional tests to ensure everything works as expected.

PR - DEV: Upgrading Discourse to Rails 6 by lis2 · Pull Request #8083 · discourse/discourse · GitHub

Cheers
Kris

22 Likes

This is awesome :confetti_ball:

Can you put this in a markdown table with percent change? Cursory look is showing not much has changes which is awesome.

Plugin wise, we have a rake task that installs all official plugins, can you run that and ensure the plugin specs pass on Rails 6? (rake plugin:spec should do the trick)

12 Likes

I updated the original post to display table. Thank you for pointing me plugins specs. I see that 2 specs failed on Travis, I will take a look and fix them.

7 Likes

There are 2 numbers here that I am finding very interesting:

RSS on 6.0 is almost 10% better.

Topic (median time) - which is our most common route is 22% faster.

This is some seriously improved perf, are you able to consistently measure 22% faster on topic-50? Can you confirm the actual page is rendering right?

11 Likes

I run benchmark 3 times and this time, results are less spectacular. My flow is to type ruby script/bench.rb in correct branch master or rails6, hit enter and don’t touch keyboard to not affect results

topic-50 RSS
5.2.3 50 322852
5.2.3 50 309684
5.2.3 50 346376
Average 50 326304
6.0.0 49 328844
6.0.0 49 321824
6.0.0 49 283584
Average 49 311417

I also connected my development server to performance database to ensure that topic page looks correct. Screenshot below looks fine to me

8 Likes

I would like to get your opinion about one fix.
I downloaded all plugins, however one new spec is failing comparing to master (./plugins/discourse-data-explorer/spec/controllers/queries_controller_spec.rb:32)

  1) DataExplorer::QueryController when disabled denies every request
     Failure/Error: render 'default/empty'

     ActionView::Template::Error:
       wrong number of arguments (given 2, expected 1)

That is fixed in master rspec-rails rspec-rails/view_rendering.rb at 4-0-dev · rspec/rspec-rails · GitHub
by changing
def self.call(_template) to def self.call(_template, _source = nil)

I can monkey patch rspec-rails with a new file in lib/freedom_patched/rspec-rails.rb but wanted to ensure that this is the best approach.

I think that this is the last change which is blocking Rails 6 from being merged.

In addition, I noticed that this spec is broken, however, it is broken on master as well, I can try to fix it (./plugins/discourse-calendar/spec/jobs/update_holiday_usernames_spec.rb:14)

 Failure/Error: expect(DiscourseCalendar.users_on_holiday).to eq([post.user.username])
       expected: ["bruce1"]
            got: []

Finally, there are deprecated methods in plugins which I can easily fix tomorrow.

What is you opinon about rspec-rails?

4 Likes

Oh my, I guess we monkey patch until rspec-rails 4 is released, can not think of any cleaner fix here.

Or… maybe … use the beta gem for now if everything is working?

5 Likes

Got you, let my try install beta tonight and see how it goes. It might be easy and smooth update.

5 Likes

I made few additional fixes.

First of all, I found why one spec was failing for me in both master and rails6 branch - FIX: Freezed time used in update_holiday_usernames_spec.rb should be UTC by lis2 · Pull Request #3 · discourse/discourse-calendar · GitHub

I also created pull requests for deprecated methods in various plugins:

I rebased latest master into rails6 branch

Finally, I updated rspec-rails to version 4.0.0.beta2 and it works fine on my local machine. Travis got some problems, however I see same problems in other pull requests so I don’t think this is correlated to upgrade of rspec-rails.

11 Likes

This is now merged :confetti_ball: :confetti_ball: :confetti_ball:

Going to keep a close eye on it today, thanks heaps for this work.

And a big thanks to the Rails team for making this such a pleasant upgrade!!

Will report back on this topic with a few pretty graphs.

16 Likes

Upgrade is looking quite uneventful, which is great. Performance is even and remains incredibly similar.

Memory and CPU look remarkably similar.

My only concern (and something I would like to get to the bottom of) it appears we get “runaway” threads for a few seconds regularly on the web workers.

So somehow, some requests are causing a huge number of threads to be spawned and then go away.

Will continue to investigate this, we need to get backtraces when the number is high, so we can find the culprit.

Given everything else is looking really good I will not be reverting the upgrade.

16 Likes

This should be fixed per:

This is a result of new code in Rails 6 that protects access to thread bound variable determining if you can use prepared statements or not.

At Discourse we do not use prepared statements at all, so this patch is not something we need.

See more at:

17 Likes

And … confirmed … my fix gets rid of the large amount of thread spikes

Also worth noting… this is how I debugged it:

  1. I wrote this little class
# frozen_string_literal: true

class Thread
  attr_accessor :origin
end

class ThreadDetective
  def self.test_thread
    Thread.new { sleep 1 }
  end
  def self.start(max_threads)
    @thread ||= Thread.new do
      self.new.monitor(max_threads)
    end

    @trace = TracePoint.new(:thread_begin) do |tp|
      Thread.current.origin = Thread.current.inspect
    end
    @trace.enable
  end

  def self.stop
    @thread&.kill
    @thread = nil
    @trace&.disable
    @trace.stop
  end

  def monitor(max_threads)
    STDERR.puts "Monitoring threads in #{Process.pid}"

    while true
      threads = Thread.list

      if threads.length > max_threads
        str = +("-" * 60)
        str << "#{threads.length} found in Process #{Process.pid}!\n"

        threads.each do |thread|
          str << "\n"
          if thread.origin
            str << thread.origin
          else
            str << thread.inspect
          end
          str << "\n"
        end
        str << ("-" * 60)

        STDERR.puts str
      end
      sleep 1
    end
  end

end
  1. I then wired in unicorn after_fork a require of this class and ran ThreadDetective.start(14)

  2. The class diligently watched every time a thread was created using a TracePoint and placed a tiny frame on the thread called origin to help me track where it came from. Once a large number of threads were observed it dumped stuff to STDERR. This can be tracked in /var/www/discourse/logs/unicorn.stderr.log

Once I knew that 100 threads were all coming from a single spot, it was very easy to isolate the root cause.

24 Likes

I noticed that I can no longer use dev.local as my hostname in development mode in Rails 6, so I’ve added an ENV variable to configure that whitelist:

9 Likes

We should not need to keep this monkey patch long term cause we just landed a fix in Rails.

3 Likes