Skip to content
This repository has been archived by the owner on Jan 15, 2024. It is now read-only.

Fix + spec for #345. #353

Merged
merged 3 commits into from
Feb 12, 2015
Merged

Fix + spec for #345. #353

merged 3 commits into from
Feb 12, 2015

Conversation

dblock
Copy link
Contributor

@dblock dblock commented Feb 12, 2015

Fix from @fedenusy for #345 + a spec.

@dblock dblock mentioned this pull request Feb 12, 2015
@fedenusy
Copy link
Contributor

Thanks man, my test writing's a little rusty.

I actually saw a couple ConnectionPool::PoolShuttingDownErrors last night, so this isn't a full-proof fix. It definitely helps though - 300+ errors went down to ~2 errors over the same period of time.

I'll think about it some more later today.

@fedenusy
Copy link
Contributor

I think the full-proof solution involves creating @down_mutex when the node gets initialized. Then:

def down?
  @down_mutex.synchronize { @down_at }
end

def down!
  @down_mutex.synchronize { @down_at = Time.new }
  @pool = nil
  @latency = nil
  Connection::Manager.shutdown(self)
end

What do you think @dblock?

@latency = nil
true
Connection::Manager.shutdown(self)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should still return true here , and not nil.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docstring says # @return [ nil ] Nothing., so we should update that as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point... if doc says that, I think it is fine to return nil

@arthurnn
Copy link
Contributor

One small comment, otherwise I will merge after

arthurnn added a commit that referenced this pull request Feb 12, 2015
@arthurnn arthurnn merged commit 265503d into mongoid:master Feb 12, 2015
@arthurnn
Copy link
Contributor

thanks

@dblock
Copy link
Contributor Author

dblock commented Feb 12, 2015

@fedenusy Clearly you need a lock to decide whether to call shutdown again, so it's a bit more than about setting down_at. I think you should try to PR that.

@fedenusy
Copy link
Contributor

Also anything that calls down? needs to to perform subsequent logic with a mutex that prevents shutdown. Might get pretty hairy.

Hopefully this fix is good enough.

@sahin
Copy link

sahin commented Feb 19, 2015

@arthurnn is this released and also added to mongoid ?

@arthurnn
Copy link
Contributor

@sahin not yet released.

@sahin
Copy link

sahin commented Feb 19, 2015

@arthurnn, it can be so good to have mongoid 4.0.3 with just a bump to the new moped

@steve-rodriguez
Copy link

@durran @arthurnn @fedenusy, when will a 2.0.4 or 2.1 version of Moped be released that contains the latest PRs to address all the primary/slave and connection pool issues? Thx!

@dblock
Copy link
Contributor Author

dblock commented Feb 20, 2015

Fyi, been running this well in production at artsy.net for the last 72 hours.

@sahin
Copy link

sahin commented Feb 20, 2015

+1

@arthurnn
Copy link
Contributor

@sahin
Copy link

sahin commented Feb 20, 2015

@arthurnn , thx a lot.

can u also update mongoid ?

@arthurnn
Copy link
Contributor

We should not need to update mongoid, as it uses Moped 2.0.x : https://github.com/mongoid/mongoid/blob/master/mongoid.gemspec#L24

a bundle update moped should be enough in your rails app.

@sahin
Copy link

sahin commented Feb 20, 2015

@arthurnn of course, we will do "bundle update moped"

but then only the people who is aware of the issue will have fix.

@InvisibleMan
Copy link

It isn't fixed.

It happens in multiple threads.
MOPED use wrong thread-safe code.
Simple test:

mongoid.yml:

development:
  sessions:
    default:
      database: test_development
      hosts:
        - localhost:27017
      options:
  options:
    preload_models: true

moped_multi_thread_fail_test.rb:

require 'mongoid'

NUMBER_THREADS = 500
ENV["MONGOID_ENV"] = 'development'

Mongoid.load!('mongoid.yml')

class Test
  include Mongoid::Document

  field :cnt, type: Integer, default: 0
end

def multi_thread_update_counters(id, thread_count)
  threads = []

  thread_count.times do |i|
    threads[i] = Thread.new do
      Test.collection.find({_id: id}).update({'$inc' => {'cnt' => 1}}, {safe: true, upsert: true})
    end
  end
  threads.each {|t| t.join }
end

multi_thread_update_counters(Test.create.id, NUMBER_THREADS)

use:

$: ruby moped_multi_thread_fail_test.rb

And 'ConnectionPool::PoolShuttingDownError':

/home/user/.rvm/gems/ruby-2.2.1/gems/connection_pool-2.1.1/lib/connection_pool/timed_stack.rb:78:in `block (2 levels) in pop': ConnectionPool::PoolShuttingDownError (ConnectionPool::PoolShuttingDownError)
    from /home/user/.rvm/gems/ruby-2.2.1/gems/connection_pool-2.1.1/lib/connection_pool/timed_stack.rb:77:in `loop'
    from /home/user/.rvm/gems/ruby-2.2.1/gems/connection_pool-2.1.1/lib/connection_pool/timed_stack.rb:77:in `block in pop'
    from /home/user/.rvm/gems/ruby-2.2.1/gems/connection_pool-2.1.1/lib/connection_pool/timed_stack.rb:76:in `synchronize'
    from /home/user/.rvm/gems/ruby-2.2.1/gems/connection_pool-2.1.1/lib/connection_pool/timed_stack.rb:76:in `pop'
    from /home/user/.rvm/gems/ruby-2.2.1/gems/connection_pool-2.1.1/lib/connection_pool.rb:78:in `checkout'
    from /home/user/.rvm/gems/ruby-2.2.1/gems/connection_pool-2.1.1/lib/connection_pool.rb:60:in `with'
    from /home/user/.rvm/gems/ruby-2.2.1/gems/moped-2.0.4/lib/moped/node.rb:114:in `connection'
    from /home/user/.rvm/gems/ruby-2.2.1/gems/moped-2.0.4/lib/moped/node.rb:140:in `disconnect'
    from /home/user/.rvm/gems/ruby-2.2.1/gems/moped-2.0.4/lib/moped/failover/disconnect.rb:26:in `execute'
    from /home/user/.rvm/gems/ruby-2.2.1/gems/moped-2.0.4/lib/moped/node.rb:185:in `rescue in ensure_connected'
    from /home/user/.rvm/gems/ruby-2.2.1/gems/moped-2.0.4/lib/moped/node.rb:188:in `ensure_connected'
    from /home/user/.rvm/gems/ruby-2.2.1/gems/moped-2.0.4/lib/moped/node.rb:589:in `block in flush'
    from /home/user/.rvm/gems/ruby-2.2.1/gems/moped-2.0.4/lib/moped/node.rb:617:in `block in logging'
    from /home/user/.rvm/gems/ruby-2.2.1/gems/activesupport-4.2.0/lib/active_support/notifications.rb:164:in `block in instrument'
    from /home/user/.rvm/gems/ruby-2.2.1/gems/activesupport-4.2.0/lib/active_support/notifications/instrumenter.rb:20:in `instrument'
    from /home/user/.rvm/gems/ruby-2.2.1/gems/activesupport-4.2.0/lib/active_support/notifications.rb:164:in `instrument'
    from /home/user/.rvm/gems/ruby-2.2.1/gems/moped-2.0.4/lib/moped/instrumentable.rb:31:in `instrument'
    from /home/user/.rvm/gems/ruby-2.2.1/gems/moped-2.0.4/lib/moped/node.rb:616:in `logging'
    from /home/user/.rvm/gems/ruby-2.2.1/gems/moped-2.0.4/lib/moped/node.rb:587:in `flush'
    from /home/user/.rvm/gems/ruby-2.2.1/gems/moped-2.0.4/lib/moped/node.rb:391:in `process'
    from /home/user/.rvm/gems/ruby-2.2.1/gems/moped-2.0.4/lib/moped/operation/read.rb:48:in `execute'
    from /home/user/.rvm/gems/ruby-2.2.1/gems/moped-2.0.4/lib/moped/node.rb:648:in `read'
    from /home/user/.rvm/gems/ruby-2.2.1/gems/moped-2.0.4/lib/moped/node.rb:90:in `command'
    from /home/user/.rvm/gems/ruby-2.2.1/gems/moped-2.0.4/lib/moped/node.rb:432:in `refresh'
    from /home/user/.rvm/gems/ruby-2.2.1/gems/moped-2.0.4/lib/moped/cluster.rb:188:in `block in refresh'
    from /home/user/.rvm/gems/ruby-2.2.1/gems/moped-2.0.4/lib/moped/cluster.rb:200:in `each'
    from /home/user/.rvm/gems/ruby-2.2.1/gems/moped-2.0.4/lib/moped/cluster.rb:200:in `refresh'
    from /home/user/.rvm/gems/ruby-2.2.1/gems/moped-2.0.4/lib/moped/cluster.rb:151:in `nodes'
    from /home/user/.rvm/gems/ruby-2.2.1/gems/moped-2.0.4/lib/moped/cluster.rb:246:in `with_primary'
    from /home/user/.rvm/gems/ruby-2.2.1/gems/moped-2.0.4/lib/moped/query.rb:426:in `update'
    from /home/user/.rvm/gems/ruby-2.2.1/gems/mongoid-4.0.2/lib/mongoid/query_cache.rb:117:in `update_with_clear_cache'

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants