-
Notifications
You must be signed in to change notification settings - Fork 151
Don't clear the auth when disconnecting, it can mess up queries on the n... #286
Don't clear the auth when disconnecting, it can mess up queries on the n... #286
Conversation
…e node that are unauthorized due to a failover.
Can you write a test case to illustrate the issue? thanks for the PR |
@arthurnn yeah I'll try to write a test for it. |
Hey @arthurnn I'm having a bit of a hard time writing this test and have hit my timebox of 45 minutes. Here's what I have so far, but the problem is that it's hard to exactly mimic the behavior: context "when an authorization error happens and the cluster gets disconnected" do
before(:all) do
@session = Support::MongoHQ.auth_session
end
let(:users) do
@session.with(safe: true)[:users]
end
before do
users.insert(documents)
end
it "retries" do
call_count0 = 0
call_count1 = 0
Moped::Protocol::Reply.any_instance.stub(:query_failed?) do
call_count0 += 1
call_count0 == 1 ? true : false
end
Moped::Protocol::Reply.any_instance.stub(:unauthorized?) do
call_count1 += 1
call_count1 == 1 ? true : false
end
query = users.find(scope: scope).sort(_id: -1)
query.one
users.database.session.cluster.nodes.each do |node|
node.stub(:connect).once.and_raise(Moped::Errors::ConnectionFailure.new)
end
users.database.session.cluster.__send__(:disconnect)
query.one.should eq documents.last
end
end the place where I think this bug was coming up is when The way in which I came up with this pull through debugging was that I added debug print statements throughout the Moped code gem on my staging servers and constantly failed over while executing queries. I was never able to reproduce it locally. |
If you think you can help with the test, that'd be great; otherwise, I think that this pull should get merged in regardless because, as-is, it definitely has the potential for problems in heavily trafficked installations. |
Don't clear the auth when disconnecting, it can mess up queries on the n...
indeed this make sense.. i will merge it for now.. and working on so good way to add a regression test for it. |
This patch for some reason immediately results in See #287 |
Had to revert this due errors happening reported from #287. |
...ode that are unauthorized due to a failover.
See #272 for backstory