Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: prevent qthrottled and qdebounced from holding strong references with bound methods #247

Merged
merged 5 commits into from
Jun 3, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 7 additions & 8 deletions .github/workflows/test_and_deploy.yml
Original file line number Diff line number Diff line change
Expand Up @@ -37,19 +37,18 @@ jobs:
- python-version: "3.11"
backend: pyside2
include:
# https://bugreports.qt.io/browse/PYSIDE-2627
- python-version: "3.10"
platform: macos-latest
backend: "'pyside6!=6.6.2'"
backend: pyside6
- python-version: "3.11"
platform: macos-latest
backend: "'pyside6!=6.6.2'"
backend: pyside6
- python-version: "3.10"
platform: windows-latest
backend: "'pyside6!=6.6.2'"
backend: pyside6
- python-version: "3.11"
platform: windows-latest
backend: "'pyside6!=6.6.2'"
backend: pyside6
- python-version: "3.12"
platform: macos-latest
backend: pyqt6
Expand All @@ -69,7 +68,7 @@ jobs:
with:
python-version: "3.8"
qt: pyqt5
pip-post-installs: 'qtpy==1.1.0 typing-extensions==3.7.4.3'
pip-post-installs: "qtpy==1.1.0 typing-extensions==3.7.4.3"
pip-install-flags: -e
coverage-upload: artifact

Expand All @@ -84,11 +83,11 @@ jobs:
with:
dependency-repo: napari/napari
dependency-ref: ${{ matrix.napari-version }}
dependency-extras: 'testing'
dependency-extras: "testing"
qt: ${{ matrix.qt }}
pytest-args: 'napari/_qt -k "not async and not qt_dims_2 and not qt_viewer_console_focus and not keybinding_editor"'
python-version: "3.10"
post-install-cmd: 'pip install lxml_html_clean'
post-install-cmd: "pip install lxml_html_clean"
strategy:
fail-fast: false
matrix:
Expand Down
3 changes: 2 additions & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,7 @@ minversion = "6.0"
testpaths = ["tests"]
filterwarnings = [
"error",
"ignore:Failed to disconnect::pytestqt",
"ignore:QPixmapCache.find:DeprecationWarning:",
"ignore:SelectableGroups dict interface:DeprecationWarning",
"ignore:The distutils package is deprecated:DeprecationWarning",
Expand Down Expand Up @@ -191,7 +192,7 @@ exclude_lines = [
"@overload",
"except ImportError",
"\\.\\.\\.",
"pass"
"pass",
]

# https://github.com/mgedmin/check-manifest#configuration
Expand Down
80 changes: 57 additions & 23 deletions src/superqt/utils/_throttler.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,12 @@

from __future__ import annotations

import weakref
from concurrent.futures import Future
from enum import IntFlag, auto
from functools import wraps
from typing import TYPE_CHECKING, Callable, Generic, TypeVar, overload
from types import MethodType
from typing import TYPE_CHECKING, Any, Callable, Generic, TypeVar, overload
from weakref import WeakKeyDictionary

from qtpy.QtCore import QObject, Qt, QTimer, Signal
Expand All @@ -53,6 +55,12 @@
P = TypeVar("P")

R = TypeVar("R")
REF_ERROR = (
"To use qthrottled or qdebounced as a method decorator, "
"objects must have `__dict__` or be weak referenceable. "
"Please either add `__weakref__` to `__slots__` or use"
"qthrottled/qdebounced as a function (not a decorator)."
)


class Kind(IntFlag):
Expand Down Expand Up @@ -157,7 +165,7 @@
self.triggered.emit()
self._timer.start()

def _maybeEmitTriggered(self, restart_timer=True) -> None:
def _maybeEmitTriggered(self, restart_timer: bool = True) -> None:
if self._hasPendingEmission:
self._emitTriggered()
if not restart_timer:
Expand Down Expand Up @@ -203,6 +211,27 @@
# below here part is unique to superqt (not from KD)


def _weak_func(func: Callable[P, R]) -> Callable[P, R]:
if isinstance(func, MethodType):
tlambert03 marked this conversation as resolved.
Show resolved Hide resolved
# this is a bound method, we need to avoid strong references
owner = func.__self__
class_func = func.__func__
try:
instance_ref = weakref.ref(owner)
except TypeError as e:
raise TypeError(REF_ERROR) from e

def weak_func(*args, **kwargs):
tlambert03 marked this conversation as resolved.
Show resolved Hide resolved
if (obj := instance_ref()) is None:
raise RuntimeError("Method called on dead object")

Check warning on line 226 in src/superqt/utils/_throttler.py

View check run for this annotation

Codecov / codecov/patch

src/superqt/utils/_throttler.py#L226

Added line #L226 was not covered by tests
tlambert03 marked this conversation as resolved.
Show resolved Hide resolved
method = class_func.__get__(obj, type(obj))
return method(*args, **kwargs)

return weak_func

return func


class ThrottledCallable(GenericSignalThrottler, Generic[P, R]):
def __init__(
self,
Expand All @@ -214,26 +243,29 @@
super().__init__(kind, emissionPolicy, parent)

self._future: Future[R] = Future()

self._is_static_method: bool = False
if isinstance(func, staticmethod):
self._func = func.__func__
else:
self._func = func
self._is_static_method = True
func = func.__func__

self.__wrapped__ = func
max_args = get_max_args(func)
self._func = _weak_func(func)
self.__wrapped__ = self._func

self._args: tuple = ()
self._kwargs: dict = {}
self.triggered.connect(self._set_future_result)
self._name = None

self._obj_dkt = WeakKeyDictionary()
self._obj_dkt: WeakKeyDictionary[Any, ThrottledCallable] = WeakKeyDictionary()

# even if we were to compile __call__ with a signature matching that of func,
# PySide wouldn't correctly inspect the signature of the ThrottledCallable
# instance: https://bugreports.qt.io/browse/PYSIDE-2423
# so we do it ourselfs and limit the number of positional arguments
# that we pass to func
self._max_args: int | None = get_max_args(self._func)
self._max_args: int | None = max_args

def __call__(self, *args: P.args, **kwargs: P.kwargs) -> "Future[R]": # noqa
if not self._future.done():
Expand All @@ -251,34 +283,31 @@
self._future.set_result(result)

def __set_name__(self, owner, name):
if not isinstance(self.__wrapped__, staticmethod):
if not self._is_static_method:
self._name = name

def _get_throttler(self, instance, owner, parent, obj):
def _get_throttler(self, instance, owner, parent, obj, name):
try:
bound_method = self._func.__get__(instance, owner)
except Exception as e: # pragma: no cover
raise RuntimeError(
f"Failed to bind function {self._func!r} to object {instance!r}"
) from e
throttler = ThrottledCallable(
self.__wrapped__.__get__(instance, owner),
bound_method,
self._kind,
self._emissionPolicy,
parent=parent,
)
throttler.setTimerType(self.timerType())
throttler.setTimeout(self.timeout())
try:
setattr(
obj,
self._name,
throttler,
)
setattr(obj, name, throttler)
except AttributeError:
try:
self._obj_dkt[obj] = throttler
except TypeError as e:
raise TypeError(
"To use qthrottled or qdebounced as a method decorator, "
"objects must have `__dict__` or be weak referenceable. "
"Please either add `__weakref__` to `__slots__` or use"
"qthrottled/qdebounced as a function (not a decorator)."
) from e
raise TypeError(REF_ERROR) from e

Check warning on line 310 in src/superqt/utils/_throttler.py

View check run for this annotation

Codecov / codecov/patch

src/superqt/utils/_throttler.py#L310

Added line #L310 was not covered by tests
return throttler

def __get__(self, instance, owner):
Expand All @@ -292,7 +321,7 @@
if parent is None and isinstance(instance, QObject):
parent = instance

return self._get_throttler(instance, owner, parent, instance)
return self._get_throttler(instance, owner, parent, instance, self._name)


@overload
Expand Down Expand Up @@ -438,6 +467,11 @@
obj = ThrottledCallable(func, kind, policy, parent=parent)
obj.setTimerType(timer_type)
obj.setTimeout(timeout)

if instance is not None:
# this is a bound method, we need to avoid strong references,
# and functools.wraps will prevent garbage collection on bound methods
tlambert03 marked this conversation as resolved.
Show resolved Hide resolved
return obj
return wraps(func)(obj)

return deco(func) if func is not None else deco
29 changes: 28 additions & 1 deletion tests/test_throttler.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import gc
import weakref
from unittest.mock import Mock

import pytest
Expand Down Expand Up @@ -116,7 +118,6 @@ def call2():
A.call2(32)

qtbot.wait(5)

assert a.count == 1
mock1.assert_called_once()
mock2.assert_called_once()
Expand Down Expand Up @@ -201,3 +202,29 @@ def func(a: int, b: int):
mock.assert_called_once_with(1, 2)
assert func.__doc__ == "docstring"
assert func.__name__ == "func"


def test_qthrottled_does_not_prevent_gc(qtbot):
mock = Mock()

class Thing:
@qdebounced(timeout=1)
def dmethod(self) -> None:
mock()

@qthrottled(timeout=1)
def tmethod(self) -> None:
mock()

thing = Thing()
thing_ref = weakref.ref(thing)
assert thing_ref() is not None
thing.dmethod()
qtbot.waitUntil(thing.dmethod._future.done, timeout=2000)
assert mock.call_count == 1
thing.tmethod()
qtbot.waitUntil(thing.tmethod._future.done, timeout=2000)
assert mock.call_count == 2
del thing
gc.collect()
assert thing_ref() is None