Thread safety problems

[sb8244]

Hello, I wanted to bring up a problem that I've recently found. The stock implementation of NetHTTPClient is not thread safe with the global lookup objects. I identified this in an older gem version, but confirmed it is also present in the latest master version.

The problem's root is demonstrated here, by the lack of thread safety to the same http instance:

http = Net::HTTP.new("github.com", 80)
threads = [
  Thread.new { p http.request(Net::HTTP::Get.new("/")) },
  Thread.new { p http.request(Net::HTTP::Get.new("/")) },
]
threads.map(&:join)

You will get a variety of errors (randomly) if you run this locally:

Errno::EBADF: Bad file descriptor
from /opt/rubies/2.2.4/lib/ruby/2.2.0/net/protocol.rb:155:in `select'

IOError: attempt to read body out of block
from /opt/rubies/2.2.4/lib/ruby/2.2.0/net/http/response.rb:330:in `stream_check'

IOError: stream closed
from /opt/rubies/2.2.4/lib/ruby/2.2.0/net/protocol.rb:153:in `read_nonblock'

Here is the reproduction with Timezone gem (master)

require 'timezone'
Timezone::Lookup.config(:geonames) do |c|
  c.username = 'your_name'
end
threads = [
  Thread.new { p Timezone.lookup(33.7489954, -84.3879824).name },
  Thread.new { p Timezone.lookup(33.7489954, -84.3879824).name },
]
threads.map(&:join)

This will provide a variety of errors as well:

Timezone::Error::GeoNames: undefined method `closed?' for nil:NilClass
	from /opt/rubies/2.2.4/lib/ruby/gems/2.2.0/gems/timezone-1.2.4/lib/timezone/lookup/geonames.rb:43:in `rescue in lookup'
	from /opt/rubies/2.2.4/lib/ruby/gems/2.2.0/gems/timezone-1.2.4/lib/timezone/lookup/geonames.rb:28:in `lookup'
	from /opt/rubies/2.2.4/lib/ruby/gems/2.2.0/gems/timezone-1.2.4/lib/timezone.rb:75:in `lookup'
	from (irb):63:in `block in irb_binding'

Proposed Fix

I'm locally going to use a wrapper around NetHTTPClient that provides thread safety. Here's a rough working version of it:

class SafeHttp
  attr_reader :protocol, :url

  def initialize(protocol, url)
    @protocol = protocol
    @url = url
  end

  def get(get_url)
    client.get(get_url)
  end

  def client
    Thread.current[:SafeHttpClient] ||= Timezone::NetHTTPClient.new(protocol, url)
  end
end

[panthomakos]

@sb8244 great find! I have a fix coming. The issue is really that the http client is cached, so the same client is used for every lookup. Using Thread.current can actually cause some issues. I was pretty consistently able to get tests to fail. I am simply going to remove the client cache so that each lookup call uses a new client - this will resolve the threadsafety issue you are seeing.

[panthomakos]

I'll target the fix for a 1.2.5 (PATCH) release.

[sb8244]

👏 nice!

For informational sake: what type of problems can arise from using Thread.current in this situation?

[panthomakos]

@sb8244 The possible problem arises because Thread.current is global in the current thread. That means that two separate lookups (ex: Timezone::Lookup::Geonames and Timezone::Lookup::Google) could end up using the same cached HTTP client.

While this is not a problem in most use-cases for the gem, it certainly arises in tests and is a possible problem if anyone has a custom http client/lookup. An example would be a custom lookup client that falls through from one client to another in the case of failure (for example if the 3rd party service is not responding).

Essentially the issue is that a single HTTP client (w/ configured credentials and URLs) could be used by two separate lookups so you could end up sending Geonames formatted queries to Google.

[sb8244]

Appreciate the explanation! Thanks for your time in maintaining this gem.

On Mon, Jan 30, 2017 at 6:25 PM Pan Thomakos ***@***.***> wrote: @sb8244 <https://github.com/sb8244> The possible problem arises because Thread.current is global in the current thread. That means that two separate lookups (ex: Timezone::Lookup::Geonames and Timezone::Lookup::Google) could end up using the same cached HTTP client. While this is not a problem in most use-cases for the gem, it certainly arises in tests and is a possible problem if anyone has a custom http client/lookup. An example would be a custom lookup client that falls through from one client to another in the case of failure (for example if the 3rd party service is not responding). Essentially the issue is that a single HTTP client (w/ configured credentials and URLs) could be used by two separate lookups so you could end up sending Geonames formatted queries to Google. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#71 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABLLK6JxRayqNbPuvVroA-Ue6eFkh9WVks5rXnFlgaJpZM4LstKF> .

-- Steve Bussey • SalesLoft • Software Engineer | e. [email protected] | w. http://salesloft.com <http://salesloft.com/?v=1&utm_expid=76406993-1.E5ncJhwyTAW9uGaR1WdBTA.1?utm_source=email-signature> | s. 3405 Piedmont Rd NE, Atlanta, GA 30305 <https://www.facebook.com/SalesLoft> <https://twitter.com/SalesLoft> <https://www.linkedin.com/company/salesloft> <http://salesloft.com/prospector/#video> <http://blog.salesloft.com/blog/>