CabbageDevelopment / qasync

Python library for using asyncio in Qt-based applications.
BSD 2-Clause "Simplified" License
307 stars 43 forks source link

Fix _QThreadWorker.run not releasing references to fulfilled command object before blocking on next queue.get call #113

Open kormax opened 3 months ago

kormax commented 3 months ago

When writing tests, I've noticed that when using qasync.QThreadExecutor, the objects, be it instance method owners, or something passed in as args, were not deleted immediately as the executor finished running the submitted task, even if there were no more references to those objects on the user side. The only way to release them was either to submit a new task, or shutdown an executor.

While investigating the code, I've also taken a look into concurrent.futures.ThreadPoolExecutor, which didn't exhibit this behaviour.

That's where I found the following code:

work_item = work_queue.get(block=True)
if work_item is not None:
    work_item.run()
    # Delete references to object. See issue16284
    del work_item

This PR mimics this change by adding a manual deletion of references to command tuple and its inner parameters inside of _QThreadWorker.run;

del command, future, callback, args, kwargs

The change can be tested by running the following example:

from qasync import QThreadExecutor
from concurrent.futures import ThreadPoolExecutor

class Demo:
    def __init__(self):
        print(f"Py ini {self}")

    def __del__(self):
        print(f"Py del {self}")

    def example(self):
        return

if __name__ == "__main__":
    threadpool_demo_obj = Demo()

    executor = ThreadPoolExecutor()
    future = executor.submit(threadpool_demo_obj.example)
    del threadpool_demo_obj
    future.result()
    print(f"Shutting down threadpool executor")
    executor.shutdown()

    qthreadpool_demo_obj = Demo()
    executor = QThreadExecutor()
    future = executor.submit(qthreadpool_demo_obj.example)
    del qthreadpool_demo_obj
    future.result()
    print(f"Shutting down qthreadpool executor")
    executor.shutdown()

Before:

Py ini <__main__.Demo object at 0x103014250>
Py del <__main__.Demo object at 0x103014250>
Shutting down threadpool executor
Py ini <__main__.Demo object at 0x103014250>
Shutting down qthreadpool executor
Py del <__main__.Demo object at 0x103014250>

After:

Py ini <__main__.Demo object at 0x104f68250>
Py del <__main__.Demo object at 0x104f68250>
Shutting down threadpool executor
Py ini <__main__.Demo object at 0x104f68250>
Py del <__main__.Demo object at 0x104f68250>
Shutting down qthreadpool executor

As for potential downsides of this change: there could theoretically be code that accidentally relies on QThreadExecutor not releasing references, especially if QObject, Slots and other Qt-related stuff is involved. But even if it's a big issue, it still might be worth adding an option to enable proper behaviour via an environment variable or other parameter.

kormax commented 3 months ago

Thinking about it, the r variable should also be set to None at the end of related block to prevent the same thing happening to the result object.