Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix threadsafety of Dynamoid::Adapter #373

Merged

Conversation

@tsub
Copy link
Contributor

commented Aug 5, 2019

I have issues

The following two types of errors occur infrequently when Dynamoid is used under multi-threaded application.

#<Thread:0x00007fc3e18e1c30@95@main.rb:13 run> terminated with exception (report_on_exception is true):
Traceback (most recent call last):
        14: from main.rb:15:in `block in safe_thread'
        13: from main.rb:6:in `watchdog'
        12: from main.rb:22:in `block (2 levels) in <main>'
        11: from main.rb:22:in `first'
        10: from /Users/tsub/sandbox/dynamoid-thread-unsafe/vendor/bundle/gems/dynamoid-3.2.0/lib/dynamoid/criteria/chain.rb:155:in `each'
         9: from /Users/tsub/sandbox/dynamoid-thread-unsafe/vendor/bundle/gems/dynamoid-3.2.0/lib/dynamoid/criteria/chain.rb:155:in `each'
         8: from /Users/tsub/sandbox/dynamoid-thread-unsafe/vendor/bundle/gems/dynamoid-3.2.0/lib/dynamoid/criteria/chain.rb:155:in `each'
         7: from /Users/tsub/sandbox/dynamoid-thread-unsafe/vendor/bundle/gems/dynamoid-3.2.0/lib/dynamoid/criteria/chain.rb:155:in `each'
         6: from /Users/tsub/sandbox/dynamoid-thread-unsafe/vendor/bundle/gems/dynamoid-3.2.0/lib/dynamoid/criteria/chain.rb:155:in `each'
         5: from /Users/tsub/sandbox/dynamoid-thread-unsafe/vendor/bundle/gems/dynamoid-3.2.0/lib/dynamoid/criteria/chain.rb:155:in `each'
         4: from /Users/tsub/sandbox/dynamoid-thread-unsafe/vendor/bundle/gems/dynamoid-3.2.0/lib/dynamoid/criteria/chain.rb:194:in `block in pages_via_query'
         3: from /Users/tsub/sandbox/dynamoid-thread-unsafe/vendor/bundle/gems/dynamoid-3.2.0/lib/dynamoid/criteria/chain.rb:194:in `each'
         2: from /Users/tsub/sandbox/dynamoid-thread-unsafe/vendor/bundle/gems/dynamoid-3.2.0/lib/dynamoid/criteria/chain.rb:194:in `each'
         1: from /Users/tsub/sandbox/dynamoid-thread-unsafe/vendor/bundle/gems/dynamoid-3.2.0/lib/dynamoid/adapter_plugin/aws_sdk_v3.rb:491:in `block in query'
/Users/tsub/sandbox/dynamoid-thread-unsafe/vendor/bundle/gems/dynamoid-3.2.0/lib/dynamoid/adapter_plugin/aws_sdk_v3.rb:603:in `describe_table': undefined method `[]' for nil:NilClass (NoMethodError)
#<Thread:0x00007fc3e1bca938@44@main.rb:13 run> terminated with exception (report_on_exception is true):
Traceback (most recent call last):
        11: from main.rb:15:in `block in safe_thread'
        10: from main.rb:6:in `watchdog'
         9: from main.rb:22:in `block (2 levels) in <main>'
         8: from main.rb:22:in `first'
         7: from /Users/tsub/sandbox/dynamoid-thread-unsafe/vendor/bundle/gems/dynamoid-3.2.0/lib/dynamoid/criteria/chain.rb:155:in `each'
         6: from /Users/tsub/sandbox/dynamoid-thread-unsafe/vendor/bundle/gems/dynamoid-3.2.0/lib/dynamoid/criteria/chain.rb:155:in `each'
         5: from /Users/tsub/sandbox/dynamoid-thread-unsafe/vendor/bundle/gems/dynamoid-3.2.0/lib/dynamoid/criteria/chain.rb:155:in `each'
         4: from /Users/tsub/sandbox/dynamoid-thread-unsafe/vendor/bundle/gems/dynamoid-3.2.0/lib/dynamoid/criteria/chain.rb:155:in `each'
         3: from /Users/tsub/sandbox/dynamoid-thread-unsafe/vendor/bundle/gems/dynamoid-3.2.0/lib/dynamoid/criteria/chain.rb:155:in `each'
         2: from /Users/tsub/sandbox/dynamoid-thread-unsafe/vendor/bundle/gems/dynamoid-3.2.0/lib/dynamoid/criteria/chain.rb:155:in `each'
         1: from /Users/tsub/sandbox/dynamoid-thread-unsafe/vendor/bundle/gems/dynamoid-3.2.0/lib/dynamoid/criteria/chain.rb:194:in `block in pages_via_query'
/Users/tsub/sandbox/dynamoid-thread-unsafe/vendor/bundle/gems/dynamoid-3.2.0/lib/dynamoid/adapter.rb:178:in `query': undefined method `query' for #<Dynamoid::AdapterPlugin::AwsSdkV3:0x00007fc3e1522e50> (NoMethodError)
Did you mean?  to_query

The multithreaded application is, for example, Sidekiq.

We have prepared an environment to reproduce this.

https://gist.github.com/tsub/72e60233ed82a8a453428ea7441e6017

You can reproduce the error as follows:

$ git clone https://gist.github.com/72e60233ed82a8a453428ea7441e6017.git reproduce-dynamoid-not-thread-safe
$ cd reproduce-dynamoid-not-thread-safe
$ docker-compose up -d
$ bundle
$ bundle exec rake dynamoid:create_tables
$ bundle exec rake dynamoid:seed
$ bundle exec ruby main.rb
...
Thread name: "20", Exception: "undefined method `query' for #<Dynamoid::AdapterPlugin::AwsSdkV3:0x00007fdaf8bf2b08>
Did you mean?  to_query"
...
Thread name: "44", Exception: "undefined method `[]' for nil:NilClass"
...

Importantly, the error is reproduced by releasing Ruby's GVL in IO processing such as puts.

100.times do |i|
  safe_thread(i.to_s) do
    puts 'debug'
    Document.where(identifier: 'hoge').first
  end
end

https://gist.github.com/tsub/72e60233ed82a8a453428ea7441e6017#file-main-rb-L18-L23

In the production environment, it can be assumed that various processing such as log output and HTTP Get request are being executed.

Related issues

How to fix

I was changed always to execute require "dynamoid/adapter_plugin/#{Dynamoid::Config.adapter}".

The reason for this error is that the result of Module#const_defined? does not match whether the code actually needs to be loaded.

For example, if you make the following changes to master

diff --git a/lib/dynamoid/adapter.rb b/lib/dynamoid/adapter.rb
index f390ecf..df2a58c 100644
--- a/lib/dynamoid/adapter.rb
+++ b/lib/dynamoid/adapter.rb
@@ -181,6 +181,13 @@ module Dynamoid
     def self.adapter_plugin_class
       unless Dynamoid.const_defined?(:AdapterPlugin) && Dynamoid::AdapterPlugin.const_defined?(Dynamoid::Config.adapter.camelcase)
         require "dynamoid/adapter_plugin/#{Dynamoid::Config.adapter}"
+      else
+        tmp_adapter = Dynamoid::AdapterPlugin.const_get(Dynamoid::Config.adapter.camelcase).new
+        puts <<~EOS
+          respond_to?(:connect!): #{tmp_adapter.respond_to?(:connect!)},
+          respond_to?(:query): #{tmp_adapter.respond_to?(:query)},
+          require: #{require "dynamoid/adapter_plugin/#{Dynamoid::Config.adapter}"}
+        EOS
       end
 
       Dynamoid::AdapterPlugin.const_get(Dynamoid::Config.adapter.camelcase)

You can see that the result of const_defined? Is true and the result of require is false (code loaded), but the actual method is not loaded.

$ bundle exec ruby main.rb
...
respond_to?(:connect!): false,
respond_to?(:query): false,
require: false
...
@andrykonchin

This comment has been minimized.

Copy link
Member

commented Aug 5, 2019

Looks great.

I need some time to play with the provided Gist.

Good job 👍

@andrykonchin andrykonchin merged commit 73a8168 into Dynamoid:master Aug 8, 2019

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@andrykonchin

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

Thank you for the fix.

@tsub tsub deleted the tsub:fix-threadsafety-of-dynamoid-adapter branch Aug 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.