instacart / makara

A Read-Write Proxy for Connections; Also provides an ActiveRecord adapter.
http://tech.taskrabbit.com/
MIT License
928 stars 170 forks source link

Proxied control methods generated do not respect arity #354

Open tjchambers opened 2 years ago

tjchambers commented 2 years ago

The metaprogrammed control methods generated by this gem are not all generated respecting the arity of the replaced method. An example in Rails 6.1.5 is that connection.owner has no arguments, but the generated methods pass an argument block and therefore fail with

[1] pry(#<Makara::ConnectionWrapper>)> connection.owner
ArgumentError: wrong number of arguments (given 1, expected 0)

The code path experienced occurred during a Mysql2 Deadlock situation. AFAICT there are no specs for the generated methods proving that they are callable.

tjchambers commented 2 years ago

I have forked the repo and will be offering a PR for consideration on the above issue.

tjchambers commented 2 years ago

Stack trace from deadlock situation for the record:

"/usr/local/bundle/gems/makara-0.5.1/lib/makara/connection_wrapper.rb:296:in `owner'",
"/usr/local/bundle/gems/activerecord-6.1.5/lib/active_record/connection_adapters/abstract/connection_pool.rb:876:in `remove_connection_from_thread_cache'",
"/usr/local/bundle/gems/activerecord-6.1.5/lib/active_record/connection_adapters/abstract/connection_pool.rb:616:in `block in remove'",
"/usr/local/lib/ruby/2.7.0/monitor.rb:202:in `synchronize'",
"/usr/local/lib/ruby/2.7.0/monitor.rb:202:in `mon_synchronize'",
"/usr/local/bundle/gems/activerecord-6.1.5/lib/active_record/connection_adapters/abstract/connection_pool.rb:615:in `remove'",
"/usr/local/bundle/gems/activerecord-6.1.5/lib/active_record/connection_adapters/abstract_adapter.rb:515:in `throw_away!'",
"/usr/local/bundle/gems/activerecord-6.1.5/lib/active_record/connection_adapters/abstract/transaction.rb:335:in `ensure in block in within_new_transaction'",
"/usr/local/bundle/gems/activerecord-6.1.5/lib/active_record/connection_adapters/abstract/transaction.rb:350:in `block in within_new_transaction'",
"/usr/local/bundle/gems/activesupport-6.1.5/lib/active_support/concurrency/load_interlock_aware_monitor.rb:26:in `block (2 levels) in synchronize'",
"/usr/local/bundle/gems/activesupport-6.1.5/lib/active_support/concurrency/load_interlock_aware_monitor.rb:25:in `handle_interrupt'",
"/usr/local/bundle/gems/activesupport-6.1.5/lib/active_support/concurrency/load_interlock_aware_monitor.rb:25:in `block in synchronize'",
"/usr/local/bundle/gems/activesupport-6.1.5/lib/active_support/concurrency/load_interlock_aware_monitor.rb:21:in `handle_interrupt'",
"/usr/local/bundle/gems/activesupport-6.1.5/lib/active_support/concurrency/load_interlock_aware_monitor.rb:21:in `synchronize'",
"/usr/local/bundle/gems/activerecord-6.1.5/lib/active_record/connection_adapters/abstract/transaction.rb:317:in `within_new_transaction'",
"/usr/local/bundle/gems/activerecord-6.1.5/lib/active_record/connection_adapters/abstract/database_statements.rb:320:in `transaction'",
"/usr/local/bundle/gems/makara-0.5.1/lib/makara/connection_wrapper.rb:249:in `block in transaction'",
"/usr/local/bundle/gems/makara-0.5.1/lib/makara/connection_wrapper.rb:198:in `_makara_hijack'",
"/usr/local/bundle/gems/makara-0.5.1/lib/makara/connection_wrapper.rb:245:in `transaction'",
"/usr/local/bundle/gems/makara-0.5.1/lib/makara/connection_wrapper.rb:105:in `method_missing'",
"/usr/local/bundle/gems/makara-0.5.1/lib/makara/proxy.rb:28:in `block (3 levels) in hijack_method'",
"/usr/local/bundle/gems/makara-0.5.1/lib/active_record/connection_adapters/makara_abstract_adapter.rb:141:in `block in appropriate_connection'",
"/usr/local/bundle/gems/makara-0.5.1/lib/makara/proxy.rb:202:in `block (3 levels) in appropriate_connection'",
"/usr/local/bundle/gems/makara-0.5.1/lib/makara/proxy.rb:266:in `hijacked'",
"/usr/local/bundle/gems/makara-0.5.1/lib/makara/proxy.rb:201:in `block (2 levels) in appropriate_connection'",
"/usr/local/bundle/gems/makara-0.5.1/lib/makara/pool.rb:109:in `block in provide'",
"/usr/local/bundle/gems/makara-0.5.1/lib/active_record/connection_adapters/makara_abstract_adapter.rb:31:in `handle'",
"/usr/local/bundle/gems/makara-0.5.1/lib/makara/pool.rb:108:in `provide'",
"/usr/local/bundle/gems/makara-0.5.1/lib/makara/proxy.rb:200:in `block in appropriate_connection'",
"/usr/local/bundle/gems/makara-0.5.1/lib/makara/proxy.rb:212:in `appropriate_pool'",
"/usr/local/bundle/gems/makara-0.5.1/lib/makara/proxy.rb:199:in `appropriate_connection'",
metatron1973 commented 2 years ago

imageplease start forensic audit of Instacart state system rack with fraud across the board stolen tips and wages daily ....

BambangSinaga commented 2 years ago

Hi @tjchambers ,

do you have workaround to solve this on? we encountered same problem since last two days..

tjchambers commented 2 years ago

Apologies, but this was done on a client project and I do not have the final code. But I believe this reflects the change to the connection_wrapper.rb. The bottomline is that the meta programming did not respect the arguments of the methods being proxied. This is a bit of a hack, but made some of the methods avoid being passed the block iirc.

      # Control methods must always be passed to the
      # Makara::Proxy control object for handling (typically
      # related to ActiveRecord connection pool management)
      @proxy.class.control_methods.each do |meth|
        method_call = RUBY_VERSION >= "3.0.0" ? "public_send(#{meth.inspect}, ...)" : "#{meth}(*args=args, block)"
        method_call2 = if meth.to_s.end_with?("=") && !meth.to_s.end_with?("==")
          method_call
        else
          method_call.dup.sub("block","&block")
        end

        extension << <<~RUBY
          def #{meth}(#{args})
            proxy = _makara
            if proxy
              proxy.control.#{method_call2}
            else
              super # Only if we are not wrapped any longer
            end
          end
        RUBY
      end
tjchambers commented 2 years ago

Obviously I failed to live up to my commitment to offer a PR, and without a set of specs it would be difficult to prove this is the best fix. However if you find this works and would offer a PR for the community I for one would appreciate it.

metatron1973 commented 2 years ago

imagereal time fraud inside the whole instacart system this is real time