From 127b04ba33263504e411c3c8eef8ed703d97fb40 Mon Sep 17 00:00:00 2001 From: Benjamin Fleischer Date: Thu, 1 Nov 2018 13:57:07 -0500 Subject: [PATCH 1/4] Add failing test for reflection thread safety bug --- test/serializers/reflection_test.rb | 52 +++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/test/serializers/reflection_test.rb b/test/serializers/reflection_test.rb index 11cb154b..7d6ea66b 100644 --- a/test/serializers/reflection_test.rb +++ b/test/serializers/reflection_test.rb @@ -423,5 +423,57 @@ module ActiveModel end # rubocop:enable Metrics/AbcSize end + class ThreadedReflectionTest < ActiveSupport::TestCase + class Post < ::Model + attributes :id, :title, :body + associations :comments + end + class Comment < ::Model + attributes :id, :body + associations :post + end + class CommentSerializer < ActiveModel::Serializer + type 'comment' + attributes :id, :body + has_one :post + end + class PostSerializer < ActiveModel::Serializer + type 'post' + attributes :id, :title, :body + has_many :comments, serializer: CommentSerializer do + sleep 0.1 + object.comments + end + end + + # per https://github.com/rails-api/active_model_serializers/issues/2270 + def test_concurrent_serialization + post1 = Post.new(id: 1, title: "Post 1 Title", body: "Post 1 Body") + post1.comments = [Comment.new(id: 1, body: "Comment on Post 1", post: post1)] + post2 = Post.new(id: 2, title: "Post 2 Title", body: "Post 2 Body") + post2.comments = [Comment.new(id: 2, body: "Comment on Post 2", post: post2)] + serialized_posts = { + first: Set.new, + second: Set.new, + } + t1 = Thread.new { + 10.times do + serialized_posts[:first] << PostSerializer.new(post1, {}).to_json + end + } + t2 = Thread.new { + 10.times do + serialized_posts[:second] << PostSerializer.new(post2, {}).to_json + end + } + t1.join + t2.join + expected_first_post_serialization = "{\"id\":1,\"title\":\"Post 1 Title\",\"body\":\"Post 1 Body\",\"comments\":[{\"id\":1,\"body\":\"Comment on Post 1\"}]}" + expected_second_post_serialization = "{\"id\":2,\"title\":\"Post 2 Title\",\"body\":\"Post 2 Body\",\"comments\":[{\"id\":2,\"body\":\"Comment on Post 2\"}]}" + + assert_equal [expected_second_post_serialization], serialized_posts[:second].to_a + assert_equal [expected_first_post_serialization], serialized_posts[:first].to_a + end + end end end From c7e847fc72dafeac1151f967b4bda2152347eea2 Mon Sep 17 00:00:00 2001 From: LongCB Date: Thu, 16 Aug 2018 10:50:36 +0700 Subject: [PATCH 2/4] Fix thread unsafe behavior --- lib/active_model/serializer.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/active_model/serializer.rb b/lib/active_model/serializer.rb index dddb465c..8452eb4e 100644 --- a/lib/active_model/serializer.rb +++ b/lib/active_model/serializer.rb @@ -347,7 +347,7 @@ module ActiveModel return Enumerator.new {} unless object Enumerator.new do |y| - self.class._reflections.each do |key, reflection| + (@reflections ||= self.class._reflections.deep_dup).each do |key, reflection| next if reflection.excluded?(self) next unless include_directive.key?(key) @@ -411,6 +411,6 @@ module ActiveModel protected - attr_accessor :instance_options + attr_accessor :instance_options, :reflections end end From b358271ef5643bfb19c07333c3fb72ad834a9105 Mon Sep 17 00:00:00 2001 From: Benjamin Fleischer Date: Thu, 1 Nov 2018 14:20:20 -0500 Subject: [PATCH 3/4] Note that we dup the entire reflection instance --- lib/active_model/serializer.rb | 4 ++-- lib/active_model/serializer/reflection.rb | 3 +++ 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/lib/active_model/serializer.rb b/lib/active_model/serializer.rb index 8452eb4e..8409cf81 100644 --- a/lib/active_model/serializer.rb +++ b/lib/active_model/serializer.rb @@ -347,7 +347,7 @@ module ActiveModel return Enumerator.new {} unless object Enumerator.new do |y| - (@reflections ||= self.class._reflections.deep_dup).each do |key, reflection| + (self.instance_reflections ||= self.class._reflections.deep_dup).each do |key, reflection| next if reflection.excluded?(self) next unless include_directive.key?(key) @@ -411,6 +411,6 @@ module ActiveModel protected - attr_accessor :instance_options, :reflections + attr_accessor :instance_options, :instance_reflections end end diff --git a/lib/active_model/serializer/reflection.rb b/lib/active_model/serializer/reflection.rb index a64aa3c4..6353933e 100644 --- a/lib/active_model/serializer/reflection.rb +++ b/lib/active_model/serializer/reflection.rb @@ -151,6 +151,9 @@ module ActiveModel # @yield [ActiveModel::Serializer] # @return [:nil, associated resource or resource collection] def value(serializer, include_slice) + # NOTE(BF): This method isn't thread-safe because the _reflections class attribute is not thread-safe + # Therefore, when we build associations from reflections, we dup the entire reflection instance. + # Better solutions much appreciated! @object = serializer.object @scope = serializer.scope From 238d7921ec54b49fbf06b1aacb4c6b1e22fd435d Mon Sep 17 00:00:00 2001 From: Benjamin Fleischer Date: Thu, 1 Nov 2018 14:51:18 -0500 Subject: [PATCH 4/4] Lint per rubocop --- test/serializers/reflection_test.rb | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/test/serializers/reflection_test.rb b/test/serializers/reflection_test.rb index 7d6ea66b..bac52deb 100644 --- a/test/serializers/reflection_test.rb +++ b/test/serializers/reflection_test.rb @@ -448,28 +448,28 @@ module ActiveModel # per https://github.com/rails-api/active_model_serializers/issues/2270 def test_concurrent_serialization - post1 = Post.new(id: 1, title: "Post 1 Title", body: "Post 1 Body") - post1.comments = [Comment.new(id: 1, body: "Comment on Post 1", post: post1)] - post2 = Post.new(id: 2, title: "Post 2 Title", body: "Post 2 Body") - post2.comments = [Comment.new(id: 2, body: "Comment on Post 2", post: post2)] + post1 = Post.new(id: 1, title: 'Post 1 Title', body: 'Post 1 Body') + post1.comments = [Comment.new(id: 1, body: 'Comment on Post 1', post: post1)] + post2 = Post.new(id: 2, title: 'Post 2 Title', body: 'Post 2 Body') + post2.comments = [Comment.new(id: 2, body: 'Comment on Post 2', post: post2)] serialized_posts = { first: Set.new, - second: Set.new, + second: Set.new } - t1 = Thread.new { + t1 = Thread.new do 10.times do serialized_posts[:first] << PostSerializer.new(post1, {}).to_json end - } - t2 = Thread.new { + end + t2 = Thread.new do 10.times do serialized_posts[:second] << PostSerializer.new(post2, {}).to_json end - } + end t1.join t2.join - expected_first_post_serialization = "{\"id\":1,\"title\":\"Post 1 Title\",\"body\":\"Post 1 Body\",\"comments\":[{\"id\":1,\"body\":\"Comment on Post 1\"}]}" - expected_second_post_serialization = "{\"id\":2,\"title\":\"Post 2 Title\",\"body\":\"Post 2 Body\",\"comments\":[{\"id\":2,\"body\":\"Comment on Post 2\"}]}" + expected_first_post_serialization = '{"id":1,"title":"Post 1 Title","body":"Post 1 Body","comments":[{"id":1,"body":"Comment on Post 1"}]}' + expected_second_post_serialization = '{"id":2,"title":"Post 2 Title","body":"Post 2 Body","comments":[{"id":2,"body":"Comment on Post 2"}]}' assert_equal [expected_second_post_serialization], serialized_posts[:second].to_a assert_equal [expected_first_post_serialization], serialized_posts[:first].to_a