Fix _QThreadWorker.run not releasing references to fulfilled command and result objects before blocking on next queue.get call

[kormax]

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]

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.

[kormax]

Hi @hosaka ,

hope you are well.

Sorry for a direct mention, but you were the last person ever active in this repository.

May I ask you if it is possible to get this PR reviewed or approved and merged?

Regards,

• Max

[hosaka]

Hello @kormax ,

I am a bit concerned about an effect this will have on QObjects, because the ownership/lifetime is managed internally in PySide at least (Shiboken). You do mention it as a "potential downside", I think this could be an issue for code that uses QObjects and Slots and most of the code using this library probably does use QObjects.

At least some tests should be performed to see if this might cause a double free.

[hosaka]

After a bit of testing with QObjects and a codebase of mine that relies on qasync I can see no issues related to this change, including parent-child objects. Just like the comments in cpython issue state, the worker should not keep these references.

@kormax it would be nice to have a new test in test_qthreadexec.py, akin to https://github.com/python/cpython/blob/main/Lib/test/test_concurrent_futures/executor.py#L80, is it something that you could add to this change?

[kormax]

NP. Gonna add this change a bit later .
(I've tried an hour ago, but for some reason when running example through pytest, something weird happens to rc/gc and test results are inconclusive, while entirely stable in normal code).

[kormax]

After looking at the problem again, it turned out that pytest or some other testing utilities affect the behavior of LogRecord, causing it to keep references to all objects in the scope where _logger.info/debug was called, thus causing new tests to fail as it prevented variables used inside of _QThreadWorker.run to be released because of reference cycles.

Disabling logging for qasync.QThreadExecutor and qasync._QThreadWorker allows the tests to pass properly.

To verify that new tests actually test the behavior, I suggest to comment out new lines added to _QThreadWorker, and observing that the tests will fail after that.

[hosaka]

Great work, thanks @kormax! I've remove autouse=true for the fixture and explicitly set it on test_no_stale_* tests.