classification
Title: test_threaded_import leaks references
Type: resource usage Stage: resolved
Components: Tests Versions: Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: matrixise, pitrou, rhettinger, serhiy.storchaka, vstinner
Priority: normal Keywords:

Created on 2017-06-08 10:57 by vstinner, last changed 2017-06-28 00:44 by vstinner. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 2001 closed vstinner, 2017-06-08 13:47
PR 2029 merged vstinner, 2017-06-09 15:09
Messages (9)
msg295403 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-06-08 10:57
http://buildbot.python.org/all/builders/x86%20Gentoo%20Refleaks%203.x/builds/28/steps/test/logs/stdio

1:50:39 load avg: 3.66 [302/405/8] test_threaded_import failed -- running: test_tarfile (80 sec)
beginning 6 repetitions
123456
......
test_threaded_import leaked [355866, 185490, 120912] references, sum=662268
test_threaded_import leaked [119138, 62100, 40480] memory blocks, sum=221718


Might be related to bpo-30598 and bpo-30547.
msg295413 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-06-08 11:38
Even when the two latest refleak fixes (ab1cb80b435a34e4f908c97cd2f3a7fe8add6505 and 865de27dd79571a4a5c7a7d22a07fb909c4a9f8e), test_threaded_import still leaks.

haypo@selma$ ./python -m test -R 3:3 -m test_side_effect_import test_threaded_import  
Run tests sequentially
0:00:00 load avg: 0.39 [1/1] test_threaded_import
beginning 6 repetitions
123456
......
test_threaded_import leaked [1374, 1374, 1374] references, sum=4122
test_threaded_import leaked [459, 460, 460] memory blocks, sum=1379
test_threaded_import failed

1 test failed:
    test_threaded_import

Total duration: 392 ms
Tests result: FAILURE
msg295421 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-06-08 12:00
The leak was introduced by the commit 346cbd351ee0dd3ab9cb9f0e4cb625556707877e.
msg295431 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-06-08 12:35
> The leak was introduced by the commit 346cbd351ee0dd3ab9cb9f0e4cb625556707877e.

This commit "bpo-16500: Allow registering at-fork handlers" adds the following code to Lib/random.py:

+if hasattr(_os, "fork"):
+    _os.register_at_fork(_inst.seed, when='child')

test_threaded_import creates fresh instance of the random module and so os.register_at_fork() is called multiple time with multiple different callbacks.

The problem is that it's not currently possible to remove callbacks. It would be nice to be able to unregister callbacks, at least a private for unit tests.

The minimum would be a os.unregister_at_fork() function.

I would prefer to have get/set functions to be able to restore callbacks once tests complete, but also to detect when an unexpected callback was registered. For example, write a new test in Lib/test/libregrtest/save_env.py for regrtest.

See also bpo-16500 which added the new API.
msg295435 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-06-08 12:44
This make the API and implementation more complex. I prefer making tests not creating several instances of the random module. Or do this in a short-living subprocess.
msg295440 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-06-08 13:33
Agreed with Serhiy.  test_threaded_import should just re-import a module which has less side effects.
msg295441 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-06-08 13:58
With https://github.com/python/cpython/pull/2001 test_threaded_import doesn't leak anymore.
msg295645 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-06-10 17:41
New changeset c5179f6e2dfcc01cf0b977b184f5b8f8ac98fab1 by Victor Stinner in branch 'master':
bpo-30599: Fix test_threaded_import reference leak (#2029)
https://github.com/python/cpython/commit/c5179f6e2dfcc01cf0b977b184f5b8f8ac98fab1
msg297086 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-06-28 00:44
test_threaded_import has been fixed, so I close the issue. Thanks Antoine and Serhiy for the reviews and feedback ;-)

I'm ok to not provide a "unregister" function. At least, until an user complains with a good rationale ;-)
History
Date User Action Args
2017-06-28 00:44:27vstinnersetstatus: open -> closed
title: os.register_at_fork(): allow to unregister callbacks -- test_threaded_import leaks references -> test_threaded_import leaks references
messages: + msg297086

resolution: fixed
stage: resolved
2017-06-10 17:41:26vstinnersetmessages: + msg295645
2017-06-09 15:09:08vstinnersetpull_requests: + pull_request2095
2017-06-08 13:58:16vstinnersetmessages: + msg295441
2017-06-08 13:47:37vstinnersetpull_requests: + pull_request2068
2017-06-08 13:33:42pitrousetmessages: + msg295440
2017-06-08 12:44:02serhiy.storchakasetnosy: + rhettinger
messages: + msg295435
2017-06-08 12:36:34matrixisesetnosy: + matrixise
2017-06-08 12:35:58vstinnersettitle: test_threaded_import leaks references -> os.register_at_fork(): allow to unregister callbacks -- test_threaded_import leaks references
2017-06-08 12:35:21vstinnersetnosy: + pitrou, serhiy.storchaka
messages: + msg295431
2017-06-08 12:00:32vstinnersetmessages: + msg295421
2017-06-08 11:38:30vstinnersetmessages: + msg295413
2017-06-08 10:57:39vstinnercreate