3 ms·

You 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

You 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

js2 5y ago
Ah, of course. Possibly this then: def _setup_transport(self): # Setup to _write() directly to the socket, and # do our own buffered reads. self._write = self.sock.sendall self._read_buffer = EMPTY_BUFFER self._quick_recv = self.sock.recv (And similarly in SSLTransport.) Then, the OSError exception raised inside Transport.close also prevents Connection.collect from destroying the Transport object. Again, I'm not disagreeing with the original PR/fix. My point is only that when the socket object is destroyed, the socket is closed. I'll bet a try/except around this (Connection.collect) would also fix it: def collect(self): if self._transport: self._transport.close() Edit: we're discussing this in two places. My apologies. For interested readers: https://github.com/celery/celery/issues/4843#issuecomment-998220198 https://github.com/celery/celery/issues/4843#issuecomment-99... I'm satisfied I understand what's going on now, and that it was a bug in celery/py-aqmp retaining the socket object due to over-broad exception handling. Python closes sockets as expected when they are released.
- postpawl 5y ago
  Yeah it should probably be setting “self._quick_recv = None” and “self._write = None” when it shuts down the transport. This probably would have fixed the leak too. I wasn’t seeing any leaky behavior after the fix though, so I think calling “socket.close()” allows GC to still clean up those other references.
  - js2 5y ago
    Right, at that point the entire Transport object is destroyed by collect.

js2 5y ago

Ah, of course. Possibly this then: def _setup_transport(self): # Setup to _write() directly to the socket, and # do our own buffered reads. self._write = self.sock.sendall self._read_buffer = EMPTY_BUFFER self._quick_recv = self.sock.recv (And similarly in SSLTransport.) Then, the OSError exception raised inside Transport.close also prevents Connection.collect from destroying the Transport object. Again, I'm not disagreeing with the original PR/fix. My point is only that when the socket object is destroyed, the socket is closed. I'll bet a try/except around this (Connection.collect) would also fix it: def collect(self): if self._transport: self._transport.close() Edit: we're discussing this in two places. My apologies. For interested readers: https://github.com/celery/celery/issues/4843#issuecomment-998220198 https://github.com/celery/celery/issues/4843#issuecomment-99... I'm satisfied I understand what's going on now, and that it was a bug in celery/py-aqmp retaining the socket object due to over-broad exception handling. Python closes sockets as expected when they are released.

postpawl 5y ago
Yeah it should probably be setting “self._quick_recv = None” and “self._write = None” when it shuts down the transport. This probably would have fixed the leak too. I wasn’t seeing any leaky behavior after the fix though, so I think calling “socket.close()” allows GC to still clean up those other references.
- js2 5y ago
  Right, at that point the entire Transport object is destroyed by collect.