Rails 6.0.0 was released 25 days ago so I think it is a time to update Discourse There were a few steps I needed to do to make that work.
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
Fix warning already initialized constant (TRADITIONAL_ESCAPED_CHAR, RFC_5987_ESCAPED_CHAR). Before dropped them I confirmed that values in ActionPack are the same
Use classic autoloader as the first step before Zeitwerk
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.
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)
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.
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
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)
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)
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.
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.
# 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
I then wired in unicorn after_fork a require of this class and ran ThreadDetective.start(14)
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.
thanks for your efforts on bringing Rails 6 to Discourse! May I humbly ask when this is expected to land in Discourse? Or is it already in 2.4.0.beta? I am just asking about the possibility whether this might break any plugin people have installed on their instances.
There were a few steps I needed to do to make that work.
