From b45383abfa598438adb1bc4d943f1ad02646b26e Mon Sep 17 00:00:00 2001 From: Sayantan Mandal Date: Tue, 23 Jun 2026 23:13:14 +0530 Subject: [PATCH] Close FTP connection and protocol on all paths in FTPDownloadHandler download_request created an FTPClient per request but never closed the connection, and the CommandFailed path returned without closing the ReceivedDataProtocol (leaking a file descriptor / memory buffer). Wrap the download in try/finally so the protocol is always closed and the connection is always torn down. Closes #7602 --- scrapy/core/downloader/handlers/ftp.py | 49 +++++++++++++------- tests/test_downloader_handler_twisted_ftp.py | 23 +++++++++ 2 files changed, 56 insertions(+), 16 deletions(-) diff --git a/scrapy/core/downloader/handlers/ftp.py b/scrapy/core/downloader/handlers/ftp.py index 6258067c1..e9883c35c 100644 --- a/scrapy/core/downloader/handlers/ftp.py +++ b/scrapy/core/downloader/handlers/ftp.py @@ -108,20 +108,37 @@ class FTPDownloadHandler(BaseDownloadHandler): client: FTPClient = await maybe_deferred_to_future( creator.connectTCP(parsed_url.hostname, parsed_url.port or 21) ) - filepath = unquote(parsed_url.path) - protocol = ReceivedDataProtocol(request.meta.get("ftp_local_filename")) + self.client: FTPClient = client try: - await maybe_deferred_to_future(client.retrieveFile(filepath, protocol)) - except CommandFailed as e: - message = str(e) - if m := _CODE_RE.search(message): - ftpcode = m.group() - httpcode = self.CODE_MAPPING.get(ftpcode, self.CODE_MAPPING["default"]) - return Response(url=request.url, status=httpcode, body=message.encode()) - raise - protocol.close() - headers = {"local filename": protocol.filename or b"", "size": protocol.size} - body = protocol.filename or protocol.body.read() - respcls = responsetypes.from_args(url=request.url, body=body) - # hints for Headers-related types may need to be fixed to not use AnyStr - return respcls(url=request.url, status=200, body=body, headers=headers) # type: ignore[arg-type] + filepath = unquote(parsed_url.path) + protocol = ReceivedDataProtocol(request.meta.get("ftp_local_filename")) + try: + await maybe_deferred_to_future(client.retrieveFile(filepath, protocol)) + except CommandFailed as e: + message = str(e) + if m := _CODE_RE.search(message): + ftpcode = m.group() + httpcode = self.CODE_MAPPING.get( + ftpcode, self.CODE_MAPPING["default"] + ) + return Response( + url=request.url, status=httpcode, body=message.encode() + ) + raise + finally: + # Always release the file descriptor / memory buffer held by + # the protocol, including on the CommandFailed path above. + protocol.close() + headers = { + "local filename": protocol.filename or b"", + "size": protocol.size, + } + body = protocol.filename or protocol.body.read() + respcls = responsetypes.from_args(url=request.url, body=body) + # hints for Headers-related types may need to be fixed to not use AnyStr + return respcls(url=request.url, status=200, body=body, headers=headers) # type: ignore[arg-type] + finally: + # Close the FTP connection instead of leaving it for the garbage + # collector to reap at some unspecified later time. + if client.transport is not None: + client.transport.loseConnection() diff --git a/tests/test_downloader_handler_twisted_ftp.py b/tests/test_downloader_handler_twisted_ftp.py index 489b70e74..359c2492b 100644 --- a/tests/test_downloader_handler_twisted_ftp.py +++ b/tests/test_downloader_handler_twisted_ftp.py @@ -109,6 +109,29 @@ class TestFTPBase(ABC): assert r.status == 404 assert r.body == b"['550 nonexistent.txt: No such file or directory.']" + @deferred_f_from_coro_f + async def test_ftp_download_closes_connection( + self, server_url: str, dh: FTPDownloadHandler + ) -> None: + # Regression test for #7602: a successful download must close the FTP + # connection rather than leaking it until garbage collection. + request = Request(url=server_url + "file.txt", meta=self.req_meta) + await dh.download_request(request) + assert dh.client.transport is not None + assert dh.client.transport.disconnecting + + @deferred_f_from_coro_f + async def test_ftp_download_closes_connection_on_missing_file( + self, server_url: str, dh: FTPDownloadHandler + ) -> None: + # Regression test for #7602: the connection must be closed even on the + # CommandFailed path (e.g. a missing file mapped to a 404 response). + request = Request(url=server_url + "nonexistent.txt", meta=self.req_meta) + r = await dh.download_request(request) + assert r.status == 404 + assert dh.client.transport is not None + assert dh.client.transport.disconnecting + @deferred_f_from_coro_f async def test_ftp_local_filename( self, server_url: str, dh: FTPDownloadHandler