Fix memory leak with pytest.raises by using weakref #2006

MSeifert04 · 2016-10-17T22:34:51Z

Checklist:

Fixes Pytest 3.0.2 memory leak with pytest.raises #1965 (Reference to bug report)
Add yourself to AUTHORS;
Add a new entry to CHANGELOG.rst
Tests: Not sure how to properly test for memory leaks. I always used some weird homebrewed functions. See for example Pytest 3.0.2 memory leak with pytest.raises #1965. Is there already some utility implemented in pytest?

This fixes the memory leak locally (#1965) but I had a lot of problems when running the tests. I hope I don't waste too much of your CI ressources if I use the PR to run the tests.

nicoddemus · 2016-10-17T23:05:30Z

Thanks a lot for the PR!

Here's a small test which fails on master and passes on your branch:

diff --git a/testing/python/raises.py b/testing/python/raises.py
index 59fd622..ba3eae2 100644
--- a/testing/python/raises.py
+++ b/testing/python/raises.py
@@ -96,3 +96,15 @@ class TestRaises:
             assert e.msg == message
         else:
             assert False, "Expected pytest.raises.Exception"
+
+    def test_raises_leak(self):
+        import weakref
+
+        class T:
+            def __call__(self):
+                raise ValueError
+
+        t = T()
+        pytest.raises(ValueError, t)
+        t = weakref.ref(t)
+        assert t() is None

On master this fails with:

F
self = <raises.TestRaises object at 0x033C8E50>

    def test_raises_leak(self):
        import weakref

        class T:
            def __call__(self):
                raise ValueError

        t = T()
        pytest.raises(ValueError, t)
        t = weakref.ref(t)
>       assert t() is None
E       assert <raises.TestRaises.test_raises_leak.<locals>.T object at 0x033C80D0> is None
E        +  where <raises.TestRaises.test_raises_leak.<locals>.T object at 0x033C80D0> = <weakref at 0x0339D8D0; to 'T' at 0x033C80D0>()

coveralls · 2016-10-17T23:05:53Z

Coverage increased (+0.0008%) to 92.85% when pulling 1248f81 on MSeifert04:fix-1965 into de16149 on pytest-dev:master.

nicoddemus · 2016-10-17T23:06:44Z

CHANGELOG.rst

@@ -9,6 +9,9 @@
 * When loading plugins, import errors which contain non-ascii messages are now properly handled in Python 2 (`#1998`_).
 Thanks `@nicoddemus`_ for the PR.

+* Fixed memory leak in the RaisesContext (pytest.raises) (`#1965`_).
+ Thanks `@MSeifert04`_ for the report the PR.


You need to add links to the issue and your user name:

.. _@MSeifert04: https://github.com/MSeifert04 .. _#1965: https://github.com/pytest-dev/pytest/issues/1965

nicoddemus

Also please add the test. 😄

nicoddemus · 2016-10-17T23:19:17Z

Hmm better yet:

diff --git a/testing/python/raises.py b/testing/python/raises.py
index 59fd622..f4222fc 100644
--- a/testing/python/raises.py
+++ b/testing/python/raises.py
@@ -96,3 +96,21 @@ class TestRaises:
             assert e.msg == message
         else:
             assert False, "Expected pytest.raises.Exception"
+
+    @pytest.mark.parametrize('method', ['function', 'with'])
+    def test_raises_leak(self, method):
+        import weakref
+
+        class T:
+            def __call__(self):
+                raise ValueError
+
+        t = T()
+        if method == 'function':
+            pytest.raises(ValueError, t)
+        else:
+            with pytest.raises(ValueError):
+                t()
+
+        t = weakref.ref(t)
+        assert t() is None

This will test both forms of using pytest.raises. 😄

MSeifert04 · 2016-10-17T23:28:00Z

@nicoddemus Thank you for the tests and how to fix the Changelog.

I confirmed that the tests failed with master and passed with this branch, so I pushed another commit.

coveralls · 2016-10-17T23:32:56Z

Coverage increased (+0.0008%) to 92.85% when pulling d418648 on MSeifert04:fix-1965 into de16149 on pytest-dev:master.

MSeifert04 · 2016-10-18T00:02:02Z

I had no memoryleaks with this branch of pytest with python 2.7 using the gc.collect-function from #1965 and I only tested the contextmanager.

But the 'method' test also fails locally with Python 2.7 (Windows 10, 64bit, conda) and passes on 3.5. Could this be a problem with the python 2.7 weakref.ref? Or am I missing something subtle?

nicoddemus · 2016-10-18T00:04:27Z

But the 'method' test also fails locally with Python 2.7 (Windows 10, 64bit, conda) and passes on 3.5. Could this be a problem with the python 2.7 weakref.ref? Or am I missing something subtle?

Locally the test_raises_memoryleak[with] test fails in Python 2.7 for me too.

I'm not sure about why is that, it should work... I will try to investigate this some more tomorrow, but if others have any ideas let's hear them. 😁

RonnyPfannschmidt

gc.collect should fix the perceived issue

RonnyPfannschmidt · 2016-10-18T05:57:23Z

testing/python/raises.py

+ with pytest.raises(ValueError):
+ t()
+
+ t = weakref.ref(t)


this one might fail on pypy and other non-ref-counted python implementations

MSeifert04 · 2016-10-18T09:47:19Z

@RonnyPfannschmidt @nicoddemus As already pointed out the issue was non-existant because I forgot to call gc.collect to free the cycles. Should I close this PR and the issue?

nicoddemus · 2016-10-18T09:57:58Z

As already pointed out the issue was non-existant because I forgot to call gc.collect to free the cycles. Should I close this PR and the issue?

You mean calling gc.collect() in your own code fixes this?

I'm not sure, I like pytest.raises not creating a circle if it doesn't have to. I detected the same problem yesterday on our code base in a case which worked fine in 2.9.2, but was leaking a reference in 3.0.3, so because of that and because the fix is simple, I think it makes sense to fix this.

What do you think @RonnyPfannschmidt?

RonnyPfannschmidt · 2016-10-18T10:01:29Z

@nicoddemus the test is incorrect correct, and also needs to call gc.collect

nicoddemus · 2016-10-18T10:08:56Z

@nicoddemus the test is incorrect correct, and also needs to call gc.collect

Hmm perhaps. But AFAIU previously (2.9.2) there was no cycle at all and gc.collect() was not needed, but in the current version a cycle is created so pytest.raises will always leak unless one calls gc.collect().

The fact that it fails only on py27 (requiring a gc.collect() call) is a mystery to me.

MSeifert04 · 2016-10-18T11:31:45Z

I have to admit I'm totally confused now. 😕

As already pointed out the issue was non-existant because I forgot to call gc.collect to free the cycles. Should I close this PR and the issue?

You mean calling gc.collect() in your own code fixes this?

Yes, it fixes the problem in my code, I haven't tested using gc.collect in the test because the CI would still fail on pypy, if I got the #2006 (comment) of @RonnyPfannschmidt right.

nicoddemus · 2016-10-18T11:40:11Z

We could skip that test on pypy... it is kind of testing an implementation detail anyway.

MSeifert04 · 2016-10-18T22:45:03Z

Using gc.collect() does not fix the test case (#2006 (comment)). I tried inserting it at several positions in the test function but on Python 2.7 the "with" test fails because the instance is still alive.

That seems to be some Python 2.7 contextmanager-weakref thingy because the following code returns a dead reference on Python 3.5 and an alive one on 2.7:

import pytest
import weakref

class TestingIsFun(object):
    def __init__(self):
        pass

    def __enter__(self, *args, **kwargs):
        pass

    def __exit__(self, *args, **kwargs):
        return True


def test_raises_memoryleak(method):
    import weakref
    class T:
        def __call__(self):
            raise ValueError
    t = T()
    if method == 'function':
        pytest.raises(ValueError, t)
    else:
        with TestingIsFun():
            t()
    t = weakref.ref(t)
    print(t)


test_raises_memoryleak('with')

just to confirm this the same behaviour is also visible with contextlib.contextmanager:

from contextlib import contextmanager

@contextmanager
def testingisnofun():
    try:
        yield
    except ValueError:
        pass

I could just skip the test for pypy and python 2.7 😄

MSeifert04 · 2016-10-25T23:39:13Z

Sorry. After it got complicated I somehow lost the motivation.

The test doesn't work because of some (unknown) Python2, weakref, contextmanagers interaction.

I wouldn't mind if any of the maintainers wants to fix it (I've set the checkbox to allow edits), or if I should just skip the with-test-part in python2 or if the PR should be closed.

nicoddemus · 2016-10-26T00:03:03Z

@MSeifert04 no worries, we appreciate the effort you put into this.

I plan to continue the work on this myself when I get the chance.

In Python 2, a context manager's __exit__() leaves sys.exc_info with the exception values even when it was supposed to suppress the exception, so we explicitly call sys.exc_clear() which removes the traceback and allow the object to be released. Also updated the test to not depend on the immediate destruction of the object but instead to ensure it is not being tracked as a cyclic reference. Fix #1965

nicoddemus · 2016-11-09T00:23:14Z

I found out the reason for the object to be kept alive in the test was that the traceback was still in sys.exc_info() even when context-manager's __exit__ method returns True, which is supposed to suppress the exception. This behavior does not happen in Python 3 so to fix the test I decided to explicitly call sys.exc_clear() to have the same behavior in both forms (function call and context-manager) as well as all python versions.

Also updated the test to not depend on the immediate destruction of the object but instead to ensure it is not being tracked as a cyclic reference (I'm curious how that will fare on PyPy).

@MSeifert04 I rebased the branch to resolve the conflicts, hope that's OK.

Applied changes myself

coveralls · 2016-11-09T00:31:21Z

Coverage decreased (-0.008%) to 92.79% when pulling 1130b9f on MSeifert04:fix-1965 into 0b94c43 on pytest-dev:master.

MSeifert04 · 2016-11-09T00:42:17Z

@nicoddemus Nice detective work 👍 Thanks for rebase and the commit.

You probably need to change the changelog to include yourself for the PR. If I remember correctly the tests were entirely your contribution and now also significant parts of the changes of the codebase.

nicoddemus · 2016-11-09T15:49:52Z

If someone else could review this in the next few days it would be great. I also plan to prepare 3.0.4 for this week as well.

RonnyPfannschmidt

well done, the final version looks good

MSeifert04 · 2016-11-09T21:22:04Z

Thanks @nicoddemus, @RonnyPfannschmidt 👍

nicoddemus · 2016-11-09T21:24:02Z

@MSeifert04 thanks as well!

nicoddemus reviewed Oct 17, 2016

View reviewed changes

nicoddemus previously requested changes Oct 17, 2016

View reviewed changes

RonnyPfannschmidt reviewed Oct 18, 2016

View reviewed changes

nicoddemus self-assigned this Oct 26, 2016

MSeifert04 and others added 3 commits November 8, 2016 22:12

Fix memory leak with pytest.raises by using weakref

1e5b21c

added test (thanks @nicoddemus) and added links in Changelog

552c7d4

RonnyPfannschmidt approved these changes Nov 9, 2016

View reviewed changes

nicoddemus merged commit fc304b8 into pytest-dev:master Nov 9, 2016

MSeifert04 deleted the fix-1965 branch November 9, 2016 21:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix memory leak with pytest.raises by using weakref #2006

Fix memory leak with pytest.raises by using weakref #2006

MSeifert04 commented Oct 17, 2016 •

edited

Loading

nicoddemus commented Oct 17, 2016

coveralls commented Oct 17, 2016

nicoddemus Oct 17, 2016

nicoddemus left a comment

nicoddemus commented Oct 17, 2016

MSeifert04 commented Oct 17, 2016

coveralls commented Oct 17, 2016

MSeifert04 commented Oct 18, 2016

nicoddemus commented Oct 18, 2016

RonnyPfannschmidt left a comment

RonnyPfannschmidt Oct 18, 2016

MSeifert04 commented Oct 18, 2016

nicoddemus commented Oct 18, 2016

RonnyPfannschmidt commented Oct 18, 2016

nicoddemus commented Oct 18, 2016

MSeifert04 commented Oct 18, 2016

nicoddemus commented Oct 18, 2016

MSeifert04 commented Oct 18, 2016

MSeifert04 commented Oct 25, 2016

nicoddemus commented Oct 26, 2016

nicoddemus commented Nov 9, 2016

coveralls commented Nov 9, 2016

MSeifert04 commented Nov 9, 2016 •

edited

Loading

nicoddemus commented Nov 9, 2016

RonnyPfannschmidt left a comment

MSeifert04 commented Nov 9, 2016

nicoddemus commented Nov 9, 2016

Fix memory leak with pytest.raises by using weakref #2006

Fix memory leak with pytest.raises by using weakref #2006

Conversation

MSeifert04 commented Oct 17, 2016 • edited Loading

nicoddemus commented Oct 17, 2016

coveralls commented Oct 17, 2016

nicoddemus Oct 17, 2016

Choose a reason for hiding this comment

nicoddemus left a comment

Choose a reason for hiding this comment

nicoddemus commented Oct 17, 2016

MSeifert04 commented Oct 17, 2016

coveralls commented Oct 17, 2016

MSeifert04 commented Oct 18, 2016

nicoddemus commented Oct 18, 2016

RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

RonnyPfannschmidt Oct 18, 2016

Choose a reason for hiding this comment

MSeifert04 commented Oct 18, 2016

nicoddemus commented Oct 18, 2016

RonnyPfannschmidt commented Oct 18, 2016

nicoddemus commented Oct 18, 2016

MSeifert04 commented Oct 18, 2016

nicoddemus commented Oct 18, 2016

MSeifert04 commented Oct 18, 2016

MSeifert04 commented Oct 25, 2016

nicoddemus commented Oct 26, 2016

nicoddemus commented Nov 9, 2016

coveralls commented Nov 9, 2016

MSeifert04 commented Nov 9, 2016 • edited Loading

nicoddemus commented Nov 9, 2016

RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

MSeifert04 commented Nov 9, 2016

nicoddemus commented Nov 9, 2016

MSeifert04 commented Oct 17, 2016 •

edited

Loading

MSeifert04 commented Nov 9, 2016 •

edited

Loading