Skip to content

Commit f53d325

Browse files
David Harshapicandocodigo
David Harsha
authored andcommitted
Fix thread safety issue in get_connection
When multiple threads are accessing a connection collection, the underlying `@collections` object can change at any point (after `connections.empty?`, `dead`, or `dead_connection.alive!`). This means the selector is not necessarily operating on a collection with an alive connection. Since selectors currently only look at alive connections, `selector.select` can return `nil` when a dead connection would be better. If you run the new test on jruby without the fix, you should see it returning `nil` and raising a `NoMethodError`: ``` NoMethodError: undefined method `dead!' for nil:NilClass <main> at /Users/dharsha/repos/elastic/elasticsearch-ruby/elasticsearch-transport/spec/elasticsearch/connections/collection_spec.rb:257 each at org/jruby/RubyEnumerator.java:396 map at org/jruby/RubyEnumerable.java:886 <main> at /Users/dharsha/repos/elastic/elasticsearch-ruby/elasticsearch-transport/spec/elasticsearch/connections/collection_spec.rb:256 ``` This switches to trying the selector first and falling back to the connection with the least errors if no alive connection is found. The fallback operates on `@connections` directly because the connections may have changed state after `selector.select` was called. Looks like this was introduced here: b5019c2
1 parent 3e06fdb commit f53d325

File tree

2 files changed

+14
-5
lines changed

2 files changed

+14
-5
lines changed

elasticsearch-transport/lib/elasticsearch/transport/transport/connections/collection.rb

+2-5
Original file line numberDiff line numberDiff line change
@@ -78,16 +78,13 @@ def all
7878

7979
# Returns a connection.
8080
#
81-
# If there are no alive connections, resurrects a connection with least failures.
81+
# If there are no alive connections, returns a connection with least failures.
8282
# Delegates to selector's `#select` method to get the connection.
8383
#
8484
# @return [Connection]
8585
#
8686
def get_connection(options={})
87-
if connections.empty? && dead_connection = dead.sort { |a,b| a.failures <=> b.failures }.first
88-
dead_connection.alive!
89-
end
90-
selector.select(options)
87+
selector.select(options) || @connections.min_by(&:failures)
9188
end
9289

9390
def each(&block)

elasticsearch-transport/spec/elasticsearch/connections/collection_spec.rb

+12
Original file line numberDiff line numberDiff line change
@@ -249,6 +249,18 @@
249249
collection.get_connection.host[:host]
250250
end).to eq((0..9).to_a)
251251
end
252+
253+
it 'always returns a connection' do
254+
threads = 20.times.map do
255+
Thread.new do
256+
20.times.map do
257+
collection.get_connection.dead!
258+
end
259+
end
260+
end
261+
262+
expect(threads.flat_map(&:value).size).to eq(400)
263+
end
252264
end
253265
end
254266
end

0 commit comments

Comments
 (0)