classification
Title: Spurious failures in test_collections in releak hunting mode after typing is imported
Type: resource usage Stage: resolved
Components: Tests Versions: Python 3.7, Python 3.6, Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: levkivskyi Nosy List: gvanrossum, haypo, levkivskyi, rhettinger, serhiy.storchaka, stutzbach
Priority: normal Keywords:

Created on 2017-02-23 23:47 by levkivskyi, last changed 2017-03-18 05:38 by serhiy.storchaka. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 469 merged levkivskyi, 2017-03-04 23:38
PR 482 merged levkivskyi, 2017-03-05 19:00
PR 483 merged levkivskyi, 2017-03-05 19:15
Messages (11)
msg288495 - (view) Author: Ivan Levkivskyi (levkivskyi) * Date: 2017-02-23 23:47
This command:

./python -c 'import runpy, typing; runpy.run_module("test")' -R 5:5 test_collections

Sometimes gives spurious failures like this:

test_collections leaked [0, 0, 3, -3, 3] memory blocks, sum=3

I think this is because ABC caches of typing.ChainMap, typing.Counter, and typing.DefaultDict are not cleared by refleak.py/dash_R_cleanup (presumably because inspect.isabstract returns False on those)

Adding a manual clean-up of these cashes to cleanup_cashes() fixes the "leak".
msg289014 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-03-05 15:11
If there are problems with ABC caches of typing.ChainMap, typing.Counter, and typing.DefaultDict, why not add corresponding clearing methods obj._abc_cache.clear and obj._abc_negative_cache.clear to typing._cleanups ?

Why there are problems only with ABC caches of these three types?
msg289015 - (view) Author: Ivan Levkivskyi (levkivskyi) * Date: 2017-03-05 15:21
> why not add corresponding clearing methods obj._abc_cache.clear and obj._abc_negative_cache.clear to typing._cleanups ?

OK, this is another possible solution. I didn't think about this, because now typing._cleanups only clear generic caches (not ABC caches).

> Why there are problems only with ABC caches of these three types?

Normally, ABC caches are cleared in dash_R_cleanup, but I think that the problem is that inspect.isabstract returns False for these three types (unlike other types in typing module), see line 147 in refleak.py:

        if not isabstract(abc):
            continue

Actually now I think that the code in dash_R_cleanup might not clear some other caches also. So maybe we just need to add all ABC caches to typing._cleanups (just in case). What do you think, Serhiy?
msg289016 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-03-05 15:45
> So maybe we just need to add all ABC caches to typing._cleanups (just in case).

I don't know. Sorry, I didn't remember exactly reasons why I didn't move the clearing of all ABC caches into clear_caches(). Perhaps I suspected that this can have unwanted consequences or decrease performance too much.

Are typing.ChainMap and others actually abstract classes? If yes, then perhaps there is something wrong with inspect.isabstract(). If no, then why ABC caches for these classes exist and non-empty?
msg289017 - (view) Author: Ivan Levkivskyi (levkivskyi) * Date: 2017-03-05 16:03
> Are typing.ChainMap and others actually abstract classes?

They are abstract classes in the sense that they are instances of abc.ABCMeta. However, for some reasons inspect checks __flags__ attribute. The latter probably reflects the fact that Deque etc. do not have abstract methods (they still need to be instances of abc.ABCMeta because they need to be generic).

I don't think that we need to change behaviour of inspect because it could be backward incompatible. I would either make changes to refleak or typing (adding few more cleanups in either place).
msg289018 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-03-05 16:12
What if explicitly set __abstractmethods__ = True for these types?
msg289019 - (view) Author: Ivan Levkivskyi (levkivskyi) * Date: 2017-03-05 16:30
> What if explicitly set __abstractmethods__ = True for these types?

Unfortunately this does not help. I think this is because dash_R_cleanup only clears caches for classes in collections.abc.__all__ and their immediate .__subclasses__(). Making this recursive (i.e. also clearing caches of __subclasses__() of __subclasses__() etc) will probably fix the problem (provided we also add __abstractmethods__ = True). However, this looks a bit too complex for me. I would rather add few more typing._cleanups (anyway this failure only happens with typing ABCs)
msg289025 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-03-05 16:57
I just wonder is there any other negative consequences? inspect.isabstract() returning False for abstract class looks a bug to me.

If you just want to add a workaround in dash_R_cleanup, I think it would be better to generate the list of all abstract classes and add three typing classes to it.

clear_caches() is called before running every test, but ABC caches are cleared not so often.
msg289032 - (view) Author: Ivan Levkivskyi (levkivskyi) * Date: 2017-03-05 17:56
> If you just want to add a workaround in dash_R_cleanup, I think it would be better to generate the list of all abstract classes and add three typing classes to it.

Yes, I just updated the PR.

I have found something else unrelated to typing, this test (in exactly this order):

./python -m test -R 5:5 test_abc test_functools

fails with a reproducible pattern:

test_functools leaked [0, 3, 1, 0, 0] memory blocks, sum=4

If you are happy with the PR now, we could open two separate issues for possible bug in inspect.isabstract and refleak in test_functools after test_abc. (the latter however might be potentially somehow related to abc caches)
msg289036 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-03-05 18:18
Thank you Ivan! Seems this has fixed issue25744. Don't you mind to backport your changes to 3.6 and 3.5?
msg289037 - (view) Author: Ivan Levkivskyi (levkivskyi) * Date: 2017-03-05 18:23
> Seems this has fixed issue25744.

This is interesting, if I remember correctly the relevant typing classes were added only recently. I will take a look at how to back-port this (probably this will require some code changes)
History
Date User Action Args
2017-03-18 05:38:56serhiy.storchakasetstatus: open -> closed
resolution: fixed
stage: resolved
2017-03-05 19:15:41levkivskyisetpull_requests: + pull_request395
2017-03-05 19:00:10levkivskyisetpull_requests: + pull_request394
2017-03-05 18:23:54levkivskyisetmessages: + msg289037
2017-03-05 18:18:15serhiy.storchakasetmessages: + msg289036
versions: + Python 3.5, Python 3.6, Python 3.7
2017-03-05 18:09:38serhiy.storchakalinkissue25744 superseder
2017-03-05 17:56:02levkivskyisetmessages: + msg289032
2017-03-05 16:57:39serhiy.storchakasetmessages: + msg289025
2017-03-05 16:30:42levkivskyisetmessages: + msg289019
2017-03-05 16:12:59serhiy.storchakasetmessages: + msg289018
2017-03-05 16:03:27levkivskyisetmessages: + msg289017
2017-03-05 15:45:50serhiy.storchakasetnosy: + rhettinger, haypo, stutzbach
messages: + msg289016
2017-03-05 15:21:32levkivskyisetmessages: + msg289015
2017-03-05 15:11:12serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg289014
2017-03-04 23:38:18levkivskyisetpull_requests: + pull_request387
2017-02-23 23:47:47levkivskyicreate