falconry / falcon

The no-magic web data plane API and microservices framework for Python developers, with a focus on reliability, correctness, and performance at scale.
https://falcon.readthedocs.io/en/stable/
Apache License 2.0
9.51k stars 937 forks source link

StaticRouteAsync leaves open files #1963

Closed vytas7 closed 2 years ago

vytas7 commented 2 years ago

When using static routes with a falcon.asgi.App, it seems that the _AsyncFileReader wrapper does not implement any .close() method, so files are left open.

On CPython, I wasn't able to demonstrate any practical impact of this bug as the file object in question is refcounted to 0 and garbage collected as soon as it goes out of scope. However, that isn't the case when running uvicorn on PyPy 3.7, as PyPy uses a different GC implementation.

Test case in point:

import io
import logging
import os.path
import unittest.mock

import falcon.asgi

logging.basicConfig(
    format='%(asctime)s [%(levelname)s] %(message)s', level=logging.INFO)

class DebugIO(io.BytesIO):

    @classmethod
    def open(cls, *args, **kwargs):
        return cls(b'Test data!\n')

    def close(self):
        logging.info(f'{self}.close()')
        super().close()

app = falcon.asgi.App()
app.add_static_route('/files', '/tmp')

debug = unittest.mock.patch('io.open', DebugIO.open)
debug.start()
vytas7 commented 2 years ago

When solving this, note that we probably need to stay on the safe side, and offload .close() to an executor, even though it is in most cases very fast. aiofiles also opts to run .close() in an executor: https://github.com/Tinche/aiofiles#usage.

s-kamil commented 2 years ago

Hey! Can I take this one?

vytas7 commented 2 years ago

Yes, you are welcome!

vytas7 commented 2 years ago

@s-kamil just checking if you are still working on fixing this bug?

vytas7 commented 2 years ago

This is now open for contributions again! (A great issue to get that last Hacktoberfest PR IMHO)