classification
Title: [2.7] Test `test_huntrleaks()` of test_regrtest fails in debug build with COUNT_ALLOCS
Type: Stage: resolved
Components: Tests Versions: Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: benjamin.peterson, cstratak, ishcherb, serhiy.storchaka, vstinner
Priority: normal Keywords: patch

Created on 2017-10-04 15:42 by ishcherb, last changed 2017-10-17 20:14 by vstinner. This issue is now closed.

Files
File name Uploaded Description Edit
00125-less-verbose-COUNT_ALLOCS.patch ishcherb, 2017-10-06 20:36
Pull Requests
URL Status Linked Edit
PR 3910 closed vstinner, 2017-10-06 19:58
PR 3927 merged vstinner, 2017-10-09 09:21
PR 4019 merged vstinner, 2017-10-17 13:55
Messages (23)
msg303705 - (view) Author: Iryna Shcherbina (ishcherb) * Date: 2017-10-04 15:42
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.
msg303821 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-10-06 14:20
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 a793037d803abf098d172f686e2b95d27863c54d 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 7e160ce356036bccda2608d9ee10bfe276dfa97a of bpo-23034. Moreover, statitics are now written into stderr rather than stdout.
msg303828 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-10-06 15:28
Commit a793037d803abf098d172f686e2b95d27863c54d 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 issue19527: 00141-fix-tests_with_COUNT_ALLOCS-v5.patch. It was not committed since issue23034 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.
msg303848 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-10-06 20:02
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(-)
msg303851 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-10-06 20:21
Do you going to backport also -X showrefcount?
msg303852 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-10-06 20:24
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 :-)
msg303855 - (view) Author: Iryna Shcherbina (ishcherb) * Date: 2017-10-06 20:36
> 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.
msg303858 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-10-06 20:47
I left this on to Benjamin, the RM of Python 2.7.
msg303859 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-10-06 20:58
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.
msg303933 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-10-09 09:22
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(-)
msg303937 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-10-09 09:38
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"
msg303944 - (view) Author: Iryna Shcherbina (ishcherb) * Date: 2017-10-09 12:26
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
msg303946 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-10-09 12:33
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?
msg303956 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-10-09 13:48
Serhiy: "Do you going to backport also -X showrefcount?"

I just created bpo-31733 "Add PYTHONSHOWREFCOUNT environment variable to Python 2.7".
msg303957 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-10-09 13:49
> 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 ;-)
msg303959 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-10-09 13:55
(I prefer to not split the discussion on this issue and my 2 PR, I prefer to only discuss the design in this issue.)

https://github.com/python/cpython/pull/3927#pullrequestreview-67963845

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.
msg303961 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-10-09 14:14
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.
msg304493 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-10-17 09:25
New changeset 7b4ba62e388474e811268322b47f80d464933541 by Victor Stinner in branch '2.7':
[2.7] bpo-31692: Add PYTHONSHOWALLOCCOUNT env var (GH-3927)
https://github.com/python/cpython/commit/7b4ba62e388474e811268322b47f80d464933541
msg304495 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-10-17 10:03
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.
msg304497 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-10-17 10:17
Isn't worth to document these changes in What's New?
msg304509 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-10-17 13:56
Serhiy: "Isn't worth to document these changes in What's New?"

Ok, I created the PR 4019.
msg304527 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-10-17 20:13
New changeset 355393e7438deeab4aeec3fcffea65e8cada083b 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)
https://github.com/python/cpython/commit/355393e7438deeab4aeec3fcffea65e8cada083b
msg304528 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-10-17 20:14
I documented the two new environment variables in What's New in Python 2.7, so this issue can now be closed again.
History
Date User Action Args
2017-10-17 20:14:08vstinnersetstatus: open -> closed
resolution: fixed
messages: + msg304528
2017-10-17 20:13:15vstinnersetmessages: + msg304527
2017-10-17 13:56:59vstinnersetstatus: closed -> open
resolution: fixed -> (no value)
messages: + msg304509
2017-10-17 13:55:57vstinnersetpull_requests: + pull_request3994
2017-10-17 10:17:44serhiy.storchakasetmessages: + msg304497
2017-10-17 10:03:02vstinnersetstatus: open -> closed
resolution: fixed
messages: + msg304495

stage: patch review -> resolved
2017-10-17 09:25:28vstinnersetmessages: + msg304493
2017-10-09 14:14:40serhiy.storchakasetmessages: + msg303961
2017-10-09 13:55:12vstinnersetmessages: + msg303959
2017-10-09 13:49:59vstinnersetmessages: + msg303957
2017-10-09 13:48:47vstinnersetmessages: + msg303956
2017-10-09 12:33:04serhiy.storchakasetmessages: + msg303946
2017-10-09 12:26:31ishcherbsetmessages: + msg303944
2017-10-09 09:38:27vstinnersetmessages: + msg303937
2017-10-09 09:22:53vstinnersetmessages: + msg303933
2017-10-09 09:21:51vstinnersetpull_requests: + pull_request3901
2017-10-06 20:58:45vstinnersetmessages: + msg303859
2017-10-06 20:47:27serhiy.storchakasetmessages: + msg303858
2017-10-06 20:36:32ishcherbsetfiles: + 00125-less-verbose-COUNT_ALLOCS.patch

messages: + msg303855
2017-10-06 20:24:33vstinnersetmessages: + msg303852
2017-10-06 20:21:23serhiy.storchakasetmessages: + msg303851
2017-10-06 20:19:52serhiy.storchakasetnosy: + benjamin.peterson
2017-10-06 20:02:02vstinnersetmessages: + msg303848
2017-10-06 19:58:09vstinnersetkeywords: + patch
stage: patch review
pull_requests: + pull_request3882
2017-10-06 15:28:02serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg303828
2017-10-06 14:20:31vstinnersetnosy: + vstinner

messages: + msg303821
title: Test `test_huntrleaks` fails in debug build with COUNT_ALLOCS -> [2.7] Test `test_huntrleaks()` of test_regrtest fails in debug build with COUNT_ALLOCS
2017-10-05 15:43:34cstrataksetnosy: + cstratak
2017-10-04 15:42:48ishcherbcreate