diff --git a/app/models/discussion_topic.rb b/app/models/discussion_topic.rb index 9a69536c494..ae1961e03cc 100644 --- a/app/models/discussion_topic.rb +++ b/app/models/discussion_topic.rb @@ -388,11 +388,16 @@ class DiscussionTopic < ActiveRecord::Base self.discussion_entries.active.count end - def unread_count(current_user = nil) + # Do not use the lock options unless you truly need + # the lock, for instance to update the count. + # Careless use has caused database transaction deadlocks + def unread_count(current_user = nil, lock: false) current_user ||= self.current_user return 0 unless current_user # default for logged out users - Shackles.activate(:master) do - topic_participant = discussion_topic_participants.lock.where(user_id: current_user).select(:unread_entry_count).first + + environment = lock ? :master : :slave + Shackles.activate(environment) do + topic_participant = discussion_topic_participants.where(user_id: current_user).select(:unread_entry_count).lock(lock).first topic_participant.try(:unread_entry_count) || self.default_unread_count end end @@ -479,7 +484,7 @@ class DiscussionTopic < ActiveRecord::Base DiscussionTopic.unique_constraint_retry do topic_participant = self.discussion_topic_participants.where(:user_id => current_user).lock.first topic_participant ||= self.discussion_topic_participants.build(:user => current_user, - :unread_entry_count => self.unread_count(current_user), + :unread_entry_count => self.unread_count(current_user, lock: true), :workflow_state => "unread", :subscribed => current_user == user && !subscription_hold(current_user, nil, nil)) topic_participant.workflow_state = opts[:new_state] if opts[:new_state] diff --git a/spec/models/discussion_topic_spec.rb b/spec/models/discussion_topic_spec.rb index c958866a168..e36808276c9 100644 --- a/spec/models/discussion_topic_spec.rb +++ b/spec/models/discussion_topic_spec.rb @@ -1244,6 +1244,23 @@ describe DiscussionTopic do end end + describe "#unread_count" do + let(:topic) do + @course.discussion_topics.create!(:title => "title", :message => "message") + end + + it "returns 0 for a nil user" do + topic.discussion_entries.create! + expect(topic.unread_count(nil)).to eq 0 + end + + it "returns the default_unread_count if the user has no discussion_topic_participant" do + topic.discussion_entries.create! + student_in_course + expect(topic.unread_count(@student)).to eq 1 + end + end + context "read/unread state" do before(:once) do @topic = @course.discussion_topics.create!(:title => "title", :message => "message", :user => @teacher)