reinh / statsd

A Ruby Statsd client that isn't a direct port of the Python example code. Because Ruby isn't Python.
MIT License
411 stars 154 forks source link

multiple calls to Statsd.new doesn't recreate socket, address family conflicts #55

Closed thwarted closed 8 years ago

thwarted commented 10 years ago

I'm using the IPv6 support from PR #46.

Because it uses only a single per-thread socket and the creation of the socket happens only once, calling .new again with a hostname that is either a literal address of another address family or a name that resolves to only an address of another address family, it stops being able to send because sendto(2) fails with EAFNOSUPPORT (Address family not supported by protocol), and stats are silently discarded.

$statsd1 = Statsd.new '127.0.0.1', 9125 # socket type is AF_INET, seen in lsof
$statsd1.increment "abc"
$statsd2 = Statsd.new '::1', 9125 # socket type should be AF_INET6, but is not
$statsd2.increment "abc" # sendto(2) errors out, seen in strace

Because Thread.current[:statsd_socket] is already defined, the object instance returned by the second call to .new, $statsd2, can't use the IPv4 socket created from the first call.

Creating multiple instances might be done if you wanted to use multiple statsd servers for different kinds of metrics. Right now, you can call .new again but it only works if the address family of the hostname in the second call is the same as the first. This problem will also come up if DNS returns either A or AAAA records (but not both) on subsequent attempts to send.

I see a couple of fixes, and which one is appropriate is based on how you intent for this library to work.

1) If you want this library to produce a singleton instance as the result of calling .new, then when .new is called, if Thread.current[:statsd_socket] is a socket, close it and set it to nil. The next time .socket is called, it will create a new socket of the appropriate address family that matches the address family of the hostname passed. The object returned is effectively (thread) global even if the variable holding the instance isn't a ruby global variable.

2) Recreate a socket of the appropriate address family on every call to send_to_socket, or perhaps only do that conditionally by storing the address family of the last created socket in thread local storage and only recreating it if the address family of the latest hostname used is a different address family.

3) Make it support multiple instances by storing the socket in the object instance instead in thread local storage. This way, each call to .new gets its own socket that matches the address family of the hostname passed in.

4) Convert to a UDP "connected" socket and store it in the object instance. Connect it at creation time and don't bother to store the hostname and port in the object instance (it would no longer be necessary). This has the added bonus of not having to do repeated DNS lookups if a hostname (vs an address) is given (negating that warning about performance and nscd in the README).

Since the socket is UDP and has no on-the-wire protocol state to be maintained (unlike, say, a database connection over TCP), there should be no problems with sharing it between threads, so the socket can be stored on the instance object rather than in thread local storage. This would make options 3 or 4 easy with less code.

The way the docs are written seem to indicate that you should be able to use multiple object instances (and you can, as long as the address family doesn't change), but the way the tests are written exploits the fact that there's only one socket created in TLS (my manipulating it directly to set up the tests).

raggi commented 10 years ago

confirm on all points.