From 4a20d09523de9cd16edd2afadc25391fc1f9109f Mon Sep 17 00:00:00 2001 From: Sam Date: Mon, 29 Jul 2013 12:25:19 +1000 Subject: [PATCH] distributed memoizer added to ensure absolute duplicate posts don't get through in case of an absolute dupe just return the memoized post This works around issues with wordpress being crazy --- app/controllers/posts_controller.rb | 47 ++++++++++++---- lib/distributed_memoizer.rb | 59 ++++++++++++++++++++ spec/components/distributed_memoizer_spec.rb | 55 ++++++++++++++++++ spec/controllers/posts_controller_spec.rb | 12 ++++ 4 files changed, 162 insertions(+), 11 deletions(-) create mode 100644 lib/distributed_memoizer.rb create mode 100644 spec/components/distributed_memoizer_spec.rb diff --git a/app/controllers/posts_controller.rb b/app/controllers/posts_controller.rb index 45081177ba5..64e1e7c25ca 100644 --- a/app/controllers/posts_controller.rb +++ b/app/controllers/posts_controller.rb @@ -1,5 +1,6 @@ require_dependency 'post_creator' require_dependency 'post_destroyer' +require_dependency 'distributed_memoizer' class PostsController < ApplicationController @@ -25,21 +26,36 @@ class PostsController < ApplicationController end def create - post_creator = PostCreator.new(current_user, create_params) - post = post_creator.create - if post_creator.errors.present? + params = create_params - # If the post was spam, flag all the user's posts as spam - current_user.flag_linked_posts_as_spam if post_creator.spam? + key = params_key(params) - render_json_error(post_creator) - else - post_serializer = PostSerializer.new(post, scope: guardian, root: false) - post_serializer.topic_slug = post.topic.slug if post.topic.present? - post_serializer.draft_sequence = DraftSequence.current(current_user, post.topic.draft_key) - render_json_dump(post_serializer) + result = DistributedMemoizer.memoize(key, 120) do + post_creator = PostCreator.new(current_user, params) + post = post_creator.create + if post_creator.errors.present? + + # If the post was spam, flag all the user's posts as spam + current_user.flag_linked_posts_as_spam if post_creator.spam? + + "e" << MultiJson.dump(errors: post_creator.errors.full_messages) + else + post_serializer = PostSerializer.new(post, scope: guardian, root: false) + post_serializer.topic_slug = post.topic.slug if post.topic.present? + post_serializer.draft_sequence = DraftSequence.current(current_user, post.topic.draft_key) + "s" << MultiJson.dump(post_serializer) + end end + + payload = result[1..-1] + if result[0] == "e" + # don't memoize errors + $redis.del(DistributedMemoizer.redis_key(key)) + render json: payload, status: 422 + else + render json: payload + end end def update @@ -193,6 +209,15 @@ class PostsController < ApplicationController private + def params_key(params) + "post##" << Digest::SHA1.hexdigest(params + .to_a + .concat([["user", current_user.id]]) + .sort{|x,y| x[0] <=> y[0]}.join do |x,y| + "#{x}:#{y}" + end) + end + def create_params permitted = [ :raw, diff --git a/lib/distributed_memoizer.rb b/lib/distributed_memoizer.rb new file mode 100644 index 00000000000..a76403fb5bb --- /dev/null +++ b/lib/distributed_memoizer.rb @@ -0,0 +1,59 @@ +class DistributedMemoizer + + # never wait for longer that 1 second for a cross process lock + MAX_WAIT = 2 + LOCK = Mutex.new + + # memoize a key across processes and machines + def self.memoize(key, duration = 60 * 60 * 24, redis = nil) + redis ||= $redis + + redis_key = self.redis_key(key) + + unless result = redis.get(redis_key) + redis_lock_key = self.redis_lock_key(key) + + start = Time.new + got_lock = false + while Time.new < start + MAX_WAIT && !got_lock + LOCK.synchronize do + got_lock = get_lock(redis,redis_lock_key) + end + sleep 0.001 + end + + unless result = redis.get(redis_key) + result = yield + redis.setex(redis_key, duration, result) + end + redis.del(redis_lock_key) + end + + result + end + + + def self.redis_lock_key(key) + "memoize_lock_" << key + end + + def self.redis_key(key) + "memoize_" << key + end + + protected + def self.get_lock(redis, redis_lock_key) + redis.watch(redis_lock_key) + current = redis.get(redis_lock_key) + return false if current + + unique = SecureRandom.hex + + result = redis.multi do + redis.setex(redis_lock_key, MAX_WAIT, unique) + end + + redis.unwatch + return result == ["OK"] + end +end diff --git a/spec/components/distributed_memoizer_spec.rb b/spec/components/distributed_memoizer_spec.rb new file mode 100644 index 00000000000..16c364feb51 --- /dev/null +++ b/spec/components/distributed_memoizer_spec.rb @@ -0,0 +1,55 @@ +require 'spec_helper' +require_dependency 'distributed_memoizer' + +describe DistributedMemoizer do + + before do + $redis.del(DistributedMemoizer.redis_key("hello")) + $redis.del(DistributedMemoizer.redis_lock_key("hello")) + $redis.unwatch + end + + # NOTE we could use a mock redis here, but I think it makes sense to test the real thing + # let(:mock_redis) { MockRedis.new } + + def memoize(&block) + DistributedMemoizer.memoize("hello", duration = 120, &block) + end + + it "returns the value of a block" do + memoize do + "abc" + end.should == "abc" + end + + it "return the old value once memoized" do + + memoize do + "abc" + end + + memoize do + "world" + end.should == "abc" + end + + it "memoizes correctly when used concurrently" do + results = [] + threads = [] + + 5.times do + threads << Thread.new do + results << memoize do + sleep 0.001 + SecureRandom.hex + end + end + end + + threads.each(&:join) + results.uniq.length.should == 1 + results.count.should == 5 + + end + +end diff --git a/spec/controllers/posts_controller_spec.rb b/spec/controllers/posts_controller_spec.rb index fa476d0c5e1..94d1f0505f4 100644 --- a/spec/controllers/posts_controller_spec.rb +++ b/spec/controllers/posts_controller_spec.rb @@ -282,6 +282,18 @@ describe PostsController do ::JSON.parse(response.body).should be_present end + it 'protects against dupes' do + # TODO we really should be using a mock redis here + xhr :post, :create, {raw: 'this is a test post 123', title: 'this is a test title 123', wpid: 1} + response.should be_success + original = response.body + + xhr :post, :create, {raw: 'this is a test post 123', title: 'this is a test title 123', wpid: 2} + response.should be_success + + response.body.should == original + end + context "errors" do let(:post_with_errors) { Fabricate.build(:post, user: user)}