3 ms·
FWIW, Python will close a socket automatically when the reference to the socket is GC'd. (Of course, you _should_ always close a socket explicitly and not rely
by js2 5y ago
FWIW, Python will close a socket automatically when the reference to the socket is GC'd. (Of course, you _should_ always close a socket explicitly and not rely on GC to handle it for you.)
So the larger issue here was that the original code was written such that when shutdown() raised an exception, the reference to the socket object wasn't released—i.e., the line of code that set `self.socket = None` was not reached.
I had to follow the link to the PR to see what was going on, because this wasn't clear to me from the minimal example in the blog post. The fix to redis-py wasn't 100% necessary. Here's the redis-py code before the PR:
https://github.com/pawl/redis-py/blob/82bad1686177c4c543818a8bfac35c6fdfc9ddf1/redis/connection.py#L715 https://github.com/pawl/redis-py/blob/82bad1686177c4c543818a...
def disconnect(self):
"Disconnects from the Redis server"
self._parser.on_disconnect()
if self._sock is None:
return
try:
if os.getpid() == self.pid:
self._sock.shutdown(socket.SHUT_RDWR)
self._sock.close()
except OSError:
pass
self._sock = None
Notice that self._sock is set to None even if shutdown() raises an exception. This will eventually cause the socket object to be GC'd, and thus closed. No memory leak here. But it's still good to call close explicitly, so I'm not disagree'ing with the PR, just that there wasn't a memory leak:
https://github.com/pawl/redis-py/commit/d7f4ac33217a95541ddf2a0e22367afb33dfac0a#diff-45b6ba995a827b484d3ef97cb806d336bf058ff9678b04cf371200ea53e584d0 https://github.com/pawl/redis-py/commit/d7f4ac33217a95541ddf...
Conversely, py-amqp looked like this:
def close(self):
if self.sock is not None:
self._shutdown_transport()
# Call shutdown first to make sure that pending messages
# reach the AMQP broker if the program exits after
# calling this method.
self.sock.shutdown(socket.SHUT_RDWR)
self.sock.close()
self.sock = None
self.connected = False
Any exception in this method prevents `self.sock = None` from running. That, _combined with the fact that_ Celery never released the py-amqp object is where the leak was.
- postpawl 5y agoI tested your theory on a branch of py-amqp: https://github.com/celery/celery/issues/4843#issuecomment-998079647 https://github.com/celery/celery/issues/4843#issuecomment-99... It still leaks if you don't close the socket.
- js2 5y agoInteresting. I conjecture this is because Python doesn’t gc the socket object immediately. Python is supposed to close file-descriptor like objects when they are gc’d, so I'd really like to better understand what's going on. Thank you for all the work so far. I'll try to figure out why my theory isn't empirically proving true. Edit: I've just done a simple test with Python 3.9.5 and it definitely closes a socket when the object is GC'd. In one terminal, I did this: nc -l 9999 In a second terminal: Python 3.9.5 (default, May 3 2021, 19:12:05) [Clang 12.0.5 (clang-1205.0.22.9)] on darwin Type "help", "copyright", "credits" or "license" for more information. >>> import socket >>> s = socket.socket() >>> s.connect(("127.0.0.1", 9999)) >>> s = None As soon as `s = None` is executed, `nc` exits. Here's what the full sequence looks like in tcpdump. First the three-way handshake (SYN, ACK, SYN) when the socket is connected: 13:09:24.448234 IP 127.0.0.1.62086 > 127.0.0.1.9999: Flags [S], seq 2594347262, win 65535, options [mss 16344,nop,wscale 6,nop,nop,TS val 62632264 ecr 0,sackOK,eol], length 0 13:09:24.448385 IP 127.0.0.1.9999 > 127.0.0.1.62086: Flags [S.], seq 682643237, ack 2594347263, win 65535, options [mss 16344,nop,wscale 6,nop,nop,TS val 3095724505 ecr 62632264,sackOK,eol], length 0 13:09:24.448410 IP 127.0.0.1.62086 > 127.0.0.1.9999: Flags [.], ack 1, win 6379, options [nop,nop,TS val 62632264 ecr 3095724505], length 0 13:09:24.448422 IP 127.0.0.1.9999 > 127.0.0.1.62086: Flags [.], ack 1, win 6379, options [nop,nop,TS val 3095724505 ecr 62632264], length 0 Then the active close when `s = None` executes: 13:09:28.234099 IP 127.0.0.1.62086 > 127.0.0.1.9999: Flags [F.], seq 1, ack 1, win 6379, options [nop,nop,TS val 62636050 ecr 3095724505], length 0 13:09:28.234178 IP 127.0.0.1.9999 > 127.0.0.1.62086: Flags [.], ack 2, win 6379, options [nop,nop,TS val 3095728291 ecr 62636050], length 0 13:09:28.234203 IP 127.0.0.1.9999 > 127.0.0.1.62086: Flags [F.], seq 1, ack 2, win 6379, options [nop,nop,TS val 3095728291 ecr 62636050], length 0 13:09:28.234240 IP 127.0.0.1.62086 > 127.0.0.1.9999: Flags [.], ack 2, win 6379, options [nop,nop,TS val 62636050 ecr 3095728291], length 0 We see the FINs from both sides along with the final ACKs. So, I don't know what the heck is causing this apparent leak. Just setting `self.sock = None` should be sufficient, modulo the vagaries of when Python GC occurs on the object.
- postpawl 5y agoYou just need 1 other reference to the socket in code to prevent it from being garbage collected without socket.close(). For example, this won't automatically close the socket: >>> s = socket.socket() >>> other_sock = s >>> s.connect(("127.0.0.1", 9999)) >>> s = None