[2.7] Test test_huntrleaks() of test_regrtest fails in debug build with COUNT_ALLOCS

[irushchyshyn]

BPO	31692
Nosy	@vstinner, @benjaminp, @serhiy-storchaka, @stratakis, @irushchyshyn
PRs	[2.7] bpo-31692: COUNT_ALLOCS now writes into stderr, fix also tests for COUNT_ALLOCS #3910 [2.7] bpo-31692: Add PYTHONSHOWALLOCCOUNT env var #3927 [2.7] bpo-31733, bpo-31692: Document 2 new env vars in What's New in Python 2.7 #4019
Files	00125-less-verbose-COUNT_ALLOCS.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2017-10-17.20:14:08.726>
created_at = <Date 2017-10-04.15:42:48.133>
labels = ['tests']
title = '[2.7] Test `test_huntrleaks()` of test_regrtest fails in debug build with COUNT_ALLOCS'
updated_at = <Date 2017-10-17.20:14:08.724>
user = 'https://github.com/irushchyshyn'

bugs.python.org fields:

activity = <Date 2017-10-17.20:14:08.724>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2017-10-17.20:14:08.726>
closer = 'vstinner'
components = ['Tests']
creation = <Date 2017-10-04.15:42:48.133>
creator = 'ishcherb'
dependencies = []
files = ['47196']
hgrepos = []
issue_num = 31692
keywords = ['patch']
message_count = 23.0
messages = ['303705', '303821', '303828', '303848', '303851', '303852', '303855', '303858', '303859', '303933', '303937', '303944', '303946', '303956', '303957', '303959', '303961', '304493', '304495', '304497', '304509', '304527', '304528']
nosy_count = 5.0
nosy_names = ['vstinner', 'benjamin.peterson', 'serhiy.storchaka', 'cstratak', 'ishcherb']
pr_nums = ['3910', '3927', '4019']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue31692'
versions = ['Python 2.7']

[irushchyshyn]

The newly added test_huntrleaks test is failing on Python 2.7.14 debug build with COUNT_ALLOCS.

======================================================================
FAIL: test_huntrleaks (test.test_regrtest.ArgsTestCase)
----------------------------------------------------------------------

Traceback (most recent call last):
  File "/builddir/build/BUILD/Python-2.7.14/Lib/test/test_regrtest.py", line 511, in test_huntrleaks
    self.check_leak(code, 'references')
  File "/builddir/build/BUILD/Python-2.7.14/Lib/test/test_regrtest.py", line 489, in check_leak
    self.assertIn(line2, output)
AssertionError: 'test_regrtest_huntrleaks leaked [1, 1, 1] references, sum=3\n' not found in 'Run tests sequentially\n0:00:00 load avg: 0.63 [1/1] test_regrtest_huntrleaks\nbeginning 6 repetitions\n123456\n......\ntest_regrtest_huntrleaks leaked [93, 93, 93] references, sum=279\n1 test failed:\n    test_regrtest_huntrleaks\n\nTotal duration: 32 ms\nTests result: FAILURE\n[53092 refs]\n'

---

On Python 2.7.14 debug build *without* COUNT_ALLOCS the test passes fine, therefore I am not sure if there is a leak detected, or is it just COUNT_ALLOCS messing with the test and it should be skipped.

[vstinner]

test_regrtest is not the single test failing when COUNT_ALLOCS is defined. Not least than 12 tests are failing.

In Python 3, many tests were fixed by skipping them if COUNT_ALLOCS is defined: commit a793037 of bpo-19527.

Since Python 3.6, even if COUNT_ALLOCS is defined, statistics are no more dumped by default, but only if -X showalloccount is defined: commit 7e160ce of bpo-23034. Moreover, statitics are now written into stderr rather than stdout.

[serhiy-storchaka]

Commit a793037 is not related to this issue. It skips the tests that were failed due to other side effect of COUNT_ALLOCS -- it makes type immortal. This is not a problem in 2.7 since all types are immortal in 2.7.

But there is other patch in bpo-19527: 00141-fix-tests_with_COUNT_ALLOCS-v5.patch. It was not committed since bpo-23034 provided better solution. You can try to backport it to 2.7, but this will just complicate the testing code for almost no benefit. Nobody wanted to do this. It was a decision to ignore this issue in Python 3.5 and earlier.

[vstinner]

I wrote PR 3910 to modify COUNT_ALLOCS: alllcations statistics are now written into stderr, rather than stdout.

The PR contains also changes to fix tests with COUNT_ALLOCS. I'm not sure about this part of the PR. I would prefer to not dump statistics by default, and add a new option to enable it.

I don't know if we can easily implement "-X showalloccount", since Python 2.7 doesn't have sys._xoptions. Maybe we could use a new environment variable instead: PYTHONSHOWALLOCCOUNT=1?

I would prefer to add a new option instead of having to patch so many unit tests:

haypo@selma$ git diff 2.7.. --stat
Lib/json/tests/test_tool.py | 8 ++++++--
Lib/test/support/init.py | 3 +++
Lib/test/test_abc.py | 2 ++
Lib/test/test_gc.py | 4 +++-
Lib/test/test_hash.py | 2 +-
Lib/test/test_module.py | 3 ++-
Lib/test/test_multiprocessing.py | 4 +++-
Lib/test/test_regrtest.py | 1 +
Lib/test/test_subprocess.py | 17 +++++++++++++++++
Lib/test/test_sys.py | 21 +++++++++++++++------
Lib/test/test_threading.py | 11 ++++++++---
Lib/test/test_tools.py | 10 +++++++---
Lib/test/test_warnings.py | 4 ++++
Lib/test/test_weakref.py | 1 +
Misc/NEWS.d/next/Core and Builtins/2017-10-06-21-56-47.bpo-31692.osJuVJ.rst | 2 ++
Python/pythonrun.c | 2 +-
16 files changed, 76 insertions(+), 19 deletions(-)

[serhiy-storchaka]

Do you going to backport also -X showrefcount?

[vstinner]

My colleague Iryna Shcherbina came to me with this issue. I'm not sure that we really need to support COUNT_ALLOCS. But strangely, it seems simpler to fix bugs rather than guessing if users like this debug mode or not :-)

There are different ways to fix tests to make them passing with COUNT_ALLOCS. As I wrote, I would prefer to add a new Python 2.7 option to explicitly asks to dump allocations statistics: add a new PYTHONSHOWALLOCCOUNT=1 environment variable for example.

What do you think Serhiy? Do you prefer to decorate many 2.7 tests to skip them if COUNT_ALLOCS is used, or to add a new option?

I should try to implement -X showalloccount :-)

[irushchyshyn]

I don't know if we can easily implement "-X showalloccount", since
Python 2.7 doesn't have sys._xoptions. Maybe we could use a new
environment variable instead: PYTHONSHOWALLOCCOUNT=1?

That is how it is bypassed in Fedora Py2 builds currently. I am attaching the patch we use.

[serhiy-storchaka]

I left this on to Benjamin, the RM of Python 2.7.

[vstinner]

Serhiy:

Do you going to backport also -X showrefcount?

I see -X as a Python3 only thing. If I have to choose, I would prefer to add a new environment variable, since it's more in the "Python2 style".

Iryna:

That is how it is bypassed in Fedora Py2 builds currently. I am attaching the patch we use.

Oh, nice to see that we get the same idea: opt-in environment variable.

I'm unhappy with the variable name: PYTHONDUMPCOUNTS seems too generic IMHO :-(

I prefer "PYTHONSHOWALLOCCOUNT" since it's closer to Python 3 "-X showalloccount" option, and "alloc" mentions that they are counters on allocations. Not just "random" counters.

[vstinner]

Ok, so. I wrote a different PR: PR 3927 adds a new PYTHONSHOWALLOCCOUNT environment variable and write allocations statistics into stderr (rather than stdout) when PYTHONSHOWALLOCCOUNT is set.

The PR also fixes the test suite for COUNT_ALLOCS.

The PR 3927 is much shorter because PYTHONSHOWALLOCCOUNT now has to be defined to dump allocations statistics.

haypo@selma$ git diff 2.7.. --stat
Doc/using/cmdline.rst | 7 +++++++
Lib/test/support/init.py | 3 +++
Lib/test/test_abc.py | 1 +
Lib/test/test_gc.py | 4 +++-
Lib/test/test_regrtest.py | 1 +
Lib/test/test_sys.py | 5 ++++-
Lib/test/test_weakref.py | 1 +
Misc/NEWS.d/next/Core and Builtins/2017-10-09-11-03-13.bpo-31692.5-bpdk.rst | 4 ++++
Python/pythonrun.c | 4 +++-
9 files changed, 27 insertions(+), 3 deletions(-)

[vstinner]

I tested PR 3927. I run "./python -m test -j8 -r" with PYTHONSHOWALLOCCOUNT unset: the full Python test suite pass in release and debug modes:

• ./configure CFLAGS="-D COUNT_ALLOCS=1"
• ./configure --with-pydebug CFLAGS="-D COUNT_ALLOCS=1"

[irushchyshyn]

I have run a test build with the patch from PR 3927 applied, and all tests passed.
Full build log can be viewed here: https://kojipkgs.fedoraproject.org//work/tasks/8392/22348392/build.log

[serhiy-storchaka]

Adding PYTHONSHOWALLOCCOUNT looks rather like a new feature to me. It is on to Bejaming to make a decision about this.

The difference between an environment variable and a command line option is that the former is inherited by children, and the latter is not (this can be considered a drawback or an advantage).

COUNT_ALLOCS is not the only option that spoils Python output. Py_TRACE_REFS adds even more noise to stderr, and this is a default option in debug build. How many tests are still left broken if just change the COUNT_ALLOCS output to stderr instead of stdout?

[vstinner]

Serhiy: "Do you going to backport also -X showrefcount?"

I just created bpo-31733 "Add PYTHONSHOWREFCOUNT environment variable to Python 2.7".

[vstinner]

How many tests are still left broken if just change the COUNT_ALLOCS output to stderr instead of stdout?

That's my first PR: PR 3910.

COUNT_ALLOCS is not the only option that spoils Python output. Py_TRACE_REFS adds even more noise to stderr, and this is a default option in debug build.

Again, I created bpo-31733 to propose to make this *super annoying* [xxx refs] line *by default*. It makes this issue more consistent ;-)

[vstinner]

(I prefer to not split the discussion on this issue and my 2 PR, I prefer to only discuss the design in this issue.)

#3927 (review)

Serhiy:

Using the requires_type_collecting decorator LGTM. But I don't sure about adding PYTHONSHOWALLOCCOUNT. This looks like a new feature to me.
What if split this PR on two parts? The one fixes issues related to immortal types, the other is about additional output.

Both changes are connected. If you only want to fix tests, that's the purpose of my much longer PR 3910.

"This looks like a new feature to me."

My PR changes the default behaviour, but COUNT_ALLOCS is not enabled by default, not even by the debug mode. You have to explicitely enable it. I expect that developers who use this option are able to track the Python 2.7 changelog and "discover" the newly added environment variable by themself. Moreover, defining PYTHONSHOWALLOCCOUNT=1 is "backward compatible" with Python < 2.7.15 and Python 3, since unknown environment variables are just ignored.

[serhiy-storchaka]

All Victor's PRs LGTM is they look good to Benjamin.

But on the next iteration we can get a report that tests don't work if set PYTHONSHOWREFCOUNT.

[vstinner]

New changeset 7b4ba62 by Victor Stinner in branch '2.7':
[2.7] bpo-31692: Add PYTHONSHOWALLOCCOUNT env var (GH-3927)
7b4ba62

[vstinner]

Python 2.7.15 will require to compile Python with COUNT_ALLOCS defined *and* to set the PYTHONSHOWALLOCCOUNT environment variable (ex: PYTHONSHOWALLOCCOUNT=1) to dump allocation counts. Moreover, counts are now dumped into stderr, instead of stdout.

Python 2.7 test suite now pass when Python is compiled with COUNT_ALLOCS.

Python 2.7 test suite doens't pass with PYTHONSHOWALLOCCOUNT set. I don't think that it's worth it to fix this case.

[serhiy-storchaka]

Isn't worth to document these changes in What's New?

[vstinner]

Serhiy: "Isn't worth to document these changes in What's New?"

Ok, I created the PR 4019.

[vstinner]

New changeset 355393e by Victor Stinner in branch '2.7':
[2.7] bpo-31733, bpo-31692: Document 2 new env vars in What's New in Python 2.7 (GH-4019)
355393e

[vstinner]

I documented the two new environment variables in What's New in Python 2.7, so this issue can now be closed again.