Changed send_to_socket method to create a new UDPSocket instance each time

[elmer-garduno]

Changed send_to_socket method to create a new UDPSocket instance per invocation. This prevents issues with thread locals and thread pools. Related to issue #34.

[reinh]

I like this! But I think we can find a cleaner way to test it. I'm tempted to use DI:

def send_to_socket(message, socket=UDPSocket.new)
  # stuff
ensure
  socket.close
end

Alternatively, we can 'stub' the socket method on the statd instance in the tests using a def:

def statsd.socket
  @socket ||= FakeUDPSocket.new
end

[reinh]

Also, I think we can take advantage of the implicit begin block provided by methods:

  def send_to_socket(message)
    self.class.logger.debug { "Statsd: #{message}" } if self.class.logger
    socket = UDPSocket.new
    socket.send(message, 0, @host, @port)
  rescue => boom
    self.class.logger.error { "Statsd: #{boom.class} #{boom}" } if self.class.logger
  ensure
    socket.close
    return nil
  end

[reinh]

I'd also like @raggi to let us know if there's any reason this won't work that I can't think of.

[elmer-garduno]

I ended up implementing a stub for get_socket on Statsd as you suggested. Let's wait for the feedback.

[reinh]

Thanks!

[reinh]

@raggi would you care to comment on this PR? I'd like to merge it but I defer to your better judgement.

[elmer-garduno]

BTW, it has been working for us without any problems, or impact on performance.

[reinh]

Thanks! I'll give @raggi some time to chime in and then go ahead and merge.

[raggi]

This will cause very large numbers of sockets instead. The current thread local implementation was added exactly to avoid the problem that this pull request will regress on. See #30

The thread local problems that folks are suffering are issues with the implementations in Fiber and there is a patch in Celluloid to prevent this from happening that is the preferred way of solving this problem. See https://github.com/celluloid/celluloid/commit/395a6dff992a16609baa94331b50c4848678967d

I would strongly prefer to wontfix this.

[raykrueger]

We fire a TON of data through statsd. Creating a new connection every time would be painful.

Perhaps, instead of the thread local, have the statsd instance just carry it's own connection and let people manage threading of the Statsd instance itself?

[raykrueger]

For instance... https://github.com/raykrueger/statsd/commit/0cf2a9619bb507b354b30d0890a1ec5aec5eb6c6

[elmer-garduno]

That sounds like the a good approach for us if you are willing to change the API.

[reinh]

I'm going to close this as a "wontfix" since per @raggi it would cause issues like #30.

I think @raykrueger's approach of letting users manage their own thread safety is a simpler, stronger approach. (It was, after all, my original approach :smile:.) We would want some clear documentation explaining the implications for threadsafety (statsd objects should not be shared between threads, etc.).