Changed channel.transport to a weakref to fix reference cycle #367
Conversation
|
Note to others that come across this: If you can't/won't apply this, calling: del channel.transportAfter closing down your channel removes the reference and allows channels to be collected. |
|
Hmm, I think this bug should be solved in another way. According to the RFC when you want to close a channel you SHOULD send an EOF, followed by a MUST send of a CHANNEL_CLOSED. The recipient of a CHANNEL_CLOSED must respond with a CHANNEL_CLOSED. In our code the flow should be: channel.close() -> send EOF and CHANNEL_CLOSED Wait for the channel to receive a CHANNEL_CLOSED -> call channel._handle_close() where we should unlink the transport. It seems that we never get to the _handle_close() method. I don't know why, but the correct fix should be triggering the unlink from there. @alex-sf what do you think? Can you see if you can find out why this isn't called with your test system? |
|
@alex-sf sorry to ping you again, but i would prefer to solve it in another way if possible, did you have a chance to look at what i wrote? :) |
|
This may be because Channel implements |
|
I think the issue is that Here is a test sftp server: import logging
import socket
import threading
import gc
import sys
from StringIO import StringIO
import paramiko
log = logging.getLogger("sftp_server")
HOST, PORT = 'localhost', 2222
HOSTKEY = """
-----BEGIN EC PRIVATE KEY-----
MHcCAQEEINnMNKzog0Cbnun+EKuFu4kcpc0iKJALw5LkC1E1x+9EoAoGCCqGSM49
AwEHoUQDQgAEt11fabDR77CPL/1OUfayQ/m3C1uRI/039pBmYagk4zQ4Mxwow6L6
UkKHL9pHHO3aWdUulY9OU88mT7O8Lg5hyA==
-----END EC PRIVATE KEY-----
"""
class AllAccessServer(paramiko.ServerInterface):
def check_auth_none(self, username):
return paramiko.AUTH_SUCCESSFUL
def check_auth_password(self, username, password):
return paramiko.AUTH_SUCCESSFUL
def check_auth_publickey(self, username, key):
return paramiko.AUTH_SUCCESSFUL
def check_channel_request(self, kind, chanid):
return paramiko.OPEN_SUCCEEDED
def start_server(host, port):
hostkey = paramiko.ECDSAKey.from_private_key(StringIO(HOSTKEY))
server_socket = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
server_socket.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, True)
server_socket.bind((host, port))
server_socket.listen(10)
while True:
conn, address = server_socket.accept()
transport = paramiko.Transport(conn)
transport.load_server_moduli()
transport.add_server_key(hostkey)
transport.set_subsystem_handler("sftp", paramiko.SFTPServer, paramiko.SFTPServerInterface)
transport.start_server(server=AllAccessServer(), event=threading.Event())
# Connect with:
# $ echo quit | sftp -oPort=2222 localhost
#
if __name__ == '__main__':
logging.basicConfig(level=logging.INFO)
try:
start_server(HOST, PORT)
except KeyboardInterrupt:
gc.collect()
for g in gc.garbage:
log.info("garbage: %r", g)
if isinstance(g, paramiko.Channel):
log.info(" g.transport.server_accepts: %r", g.transport.server_accepts)
log.info(" g.transport._channels.values(): %r", g.transport._channels.values())
sys.exit(0)Connect and cleanly disconnect to this a number of times via Then kill the server with Ctrl-C and it will dump uncollectable garbage. After connecting/disconnecting a few times we have some uncollectable All of the Calling |
We're using a number of Paramiko-based SSH proxies and all of them were leaking multiple gigabytes of RAM per day. After some investigation, I noticed Channel objects were not being collected properly to a reference cycle:
I pared down our server code considerably and sent a couple hundred of requests through the app with a stock 1.13 package to demonstrate the leak:
http://snapfiber.com/ssh-server_paramiko-1-13-unpatched.svg
After applying this change, the leak no longer exists:
http://snapfiber.com/ssh-server_paramiko-1-13-patched.svg