Socket creation in DogStatsd class is not thread safe.

[GrahamDumpleton]

The code in get_socket() is:

    def get_socket(self):
        '''
        Return a connected socket
        '''
        if not self.socket:
            self.socket = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
            self.socket.connect((self.host, self.port))
        return self.socket

This code is not thread safe and can result in failure in a multithreaded application, with potential loss of initial metric data points on process startup.

The function which triggers the function is:

    def _send_to_server(self, packet):
        try:
            # If set, use socket directly
            (self.socket or self.get_socket()).send(packet.encode(self.encoding))
        except socket.error:
            log.info("Error submitting packet, will try refreshing the socket")
            self.socket = None
            try:
                self.get_socket().send(packet.encode(self.encoding))
            except socket.error:
                log.exception("Failed to send packet with a newly binded socket")

Specifically, the line:

            (self.socket or self.get_socket()).send(packet.encode(self.encoding))

Because in get_socket() the newly created socket is assigned direct to self.socket, then a separate thread calling into _send_to_server() can see the socket before the socket has been connected.

The end result is a socket error:

error: [Errno 39] Destination address required

in the second thread which can call send() on the unconnected socket.

This triggers the socket.error exception clause which wipes out self.socket and sets it to None.

The original thread when it returns from connect() on the socket then returns None as a socket which the first thread then calls send() on with the resulting exception of:

[Sun Jun 07 19:28:26.101636 2015] [wsgi:error] [pid 89948:tid 4361912320]   File ".../datadog/dogstatsd/base.py", line 189, in _send_to_server
[Sun Jun 07 19:28:26.101660 2015] [wsgi:error] [pid 89948:tid 4361912320]     self.get_socket().send(packet.encode(self.encoding))
[Sun Jun 07 19:28:26.101681 2015] [wsgi:error] [pid 89948:tid 4361912320] AttributeError: 'NoneType' object has no attribute 'send'

To avoid this problem, get_socket() should be rewritten as:

    def get_socket(self):
        '''
        Return a connected socket
        '''
        if not self.socket:
            sock = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
            sock.connect((self.host, self.port))
            self.socket = sock
        return self.socket

In other words, assign the socket to a local variable and do the connect() before then assigning it to the class instance.

Note that although this avoids the presenting problem, the code still has a thread race condition due to no thread locking being used. That is, multiple threads could create the socket connection with only one winning out. This will not cause any issue except that an extra socket connection will temporarily exist until it is closed through reference count reduction in CPython or garbage collection in pypy.

This race condition is tolerable, although with the way that checks are done it would be trivial to make socket creation entirely thread safe without introducing thread lock contention beyond the initial point of creation the first time it is required.

[yannmh]

Thanks a lot for the very detailed explanations @GrahamDumpleton !

I opened a new PR following your recommandations #60.