Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add a nomemory_allocator to the _testcapi module #74880

Closed
xdegaye mannequin opened this issue Jun 18, 2017 · 19 comments
Closed

add a nomemory_allocator to the _testcapi module #74880

xdegaye mannequin opened this issue Jun 18, 2017 · 19 comments
Labels
3.7 (EOL) end of life tests Tests in the Lib/test dir type-feature A feature request or enhancement

Comments

@xdegaye
Copy link
Mannequin

xdegaye mannequin commented Jun 18, 2017

BPO 30695
Nosy @vstinner, @xdegaye, @serhiy-storchaka
PRs
  • bpo-30695: Add set_nomemory(start, stop) to _testcapi #2406
  • [3.6] bpo-30695: Add set_nomemory(start, stop) to _testcapi (GH-2406) #4083
  • Files
  • nomemory_allocator.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-23.13:24:32.179>
    created_at = <Date 2017-06-18.10:21:11.337>
    labels = ['3.7', 'type-feature', 'tests']
    title = 'add a nomemory_allocator to the _testcapi module'
    updated_at = <Date 2017-10-23.13:24:32.179>
    user = 'https://github.com/xdegaye'

    bugs.python.org fields:

    activity = <Date 2017-10-23.13:24:32.179>
    actor = 'xdegaye'
    assignee = 'none'
    closed = True
    closed_date = <Date 2017-10-23.13:24:32.179>
    closer = 'xdegaye'
    components = ['Tests']
    creation = <Date 2017-06-18.10:21:11.337>
    creator = 'xdegaye'
    dependencies = []
    files = ['46958']
    hgrepos = []
    issue_num = 30695
    keywords = ['patch']
    message_count = 19.0
    messages = ['296266', '296276', '296299', '296318', '296320', '296323', '296331', '296333', '296338', '296339', '296340', '296889', '296893', '296894', '296900', '297412', '297481', '304797', '304802']
    nosy_count = 3.0
    nosy_names = ['vstinner', 'xdegaye', 'serhiy.storchaka']
    pr_nums = ['2406', '4083']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue30695'
    versions = ['Python 3.6', 'Python 3.7']

    @xdegaye
    Copy link
    Mannequin Author

    xdegaye mannequin commented Jun 18, 2017

    Add the set_nomemory_allocator() function to _testcapi that sets a no memory allocator.

    @xdegaye xdegaye mannequin added 3.7 (EOL) end of life tests Tests in the Lib/test dir type-feature A feature request or enhancement labels Jun 18, 2017
    @serhiy-storchaka
    Copy link
    Member

    I think the allocators must just return NULL, without setting the error.

    @vstinner
    Copy link
    Member

    Nice. Do you know my https://pypi.python.org/pypi/pyfailmalloc project? It
    helped me to identify and fix dozens of code which didn't handle properly
    memory allocation failures. Seach for "pyfailmalloc" in the bug tracker
    (closed issues) ;-)

    @xdegaye
    Copy link
    Mannequin Author

    xdegaye mannequin commented Jun 19, 2017

    Yes, actually I thought first about re-opening bpo-19817 instead of opening this new issue.

    @xdegaye
    Copy link
    Mannequin Author

    xdegaye mannequin commented Jun 19, 2017

    I think the allocators must just return NULL, without setting the error.
    You are right, thanks.

    @vstinner
    Copy link
    Member

    The main difference between your change and pyfailmalloc is the ability in pyfailmalloc to only fail in N allocations. If you want to test various code paths, you need to allow N allocations to reach the deep code that you want to test.

    My code is something like 100 lines of C code.
    https://bitbucket.org/haypo/pyfailmalloc/src/cdaf3609a30b88243d04e79d6074f3b28d9b64e3/failmalloc.c?at=default&fileviewer=file-view-default

    IMHO it's small enough to fit directly into _testcapi.

    The question is more what do you want to do with it? It was proposed to "include" pyfailmalloc directly in the Python test suite. Maybe add an option to enable it?

    I ran the Python test suite with pyfailmalloc in gdb using this script:
    https://bitbucket.org/haypo/pyfailmalloc/src/cdaf3609a30b88243d04e79d6074f3b28d9b64e3/debug_cpython.gdb?at=default&fileviewer=file-view-default

    ./python -m test -F -r -v -x test_tracemalloc

    I waited for the next *crash* and ignored all expected tests failing with MemoryError. --forever (-F) is nice to run random tests until you get a crash.

    @xdegaye
    Copy link
    Mannequin Author

    xdegaye mannequin commented Jun 19, 2017

    The chain of events following a call to set_nomemory_allocator() is deterministic, this is another difference with pyfailmalloc. Also what happens then after this call is not very close to real life as memory is never freed.

    Having the possibility to trigger memory exhaustion in N allocations instead of rightaway would be very useful. Maybe this could be done by including pyfailmalloc in _testcapi with some changes to enable multiple behaviors, among which one of them would be deterministic.

    @vstinner
    Copy link
    Member

    Your code can be reproduced with pyfailmalloc using N=0 :-)

    @xdegaye
    Copy link
    Mannequin Author

    xdegaye mannequin commented Jun 19, 2017

    I am not sure, failmalloc.enable(range) accepts only range > 1, hook_malloc() fails only once instead of permanently and hook_free() does free memory.

    @vstinner
    Copy link
    Member

    I am not sure, failmalloc.enable(range) accepts only range > 1, hook_malloc() fails only once instead of permanently and hook_free() does free memory.

    All these things can change :-)

    Also what happens then after this call is not very close to real life as memory is never freed.

    Why not freeing memory?

    @xdegaye
    Copy link
    Mannequin Author

    xdegaye mannequin commented Jun 19, 2017

    Good point. Not freeing the memory made the implementation of set_nomemory_allocator() simpler, no need to call PyMem_GetAllocator(). Forget about this point, sorry :-)

    @vstinner
    Copy link
    Member

    I prefer your new PR. But is it useful to test N memory allocation failures in a row? The API and the code would be simpler if we would only test a single failure. What do you think?

    I know that pyfailmalloc is different, but I'm not sure that pyfailmalloc design is right neither :-)

    @xdegaye
    Copy link
    Mannequin Author

    xdegaye mannequin commented Jun 26, 2017

    For the record, while working on the test case of PR 2406, I found by chance that the following script:

    # Script start.
    import _testcapi
    class C(): pass
    _testcapi.set_nomemory(0, 5)
    C()
    # Script end.

    fails with:

    python: Objects/call.c:89: _PyObject_FastCallDict: Assertion `!PyErr_Occurred()' failed.
    Aborted (core dumped)

    I will create a new issue when the current issue is closed.

    @xdegaye
    Copy link
    Mannequin Author

    xdegaye mannequin commented Jun 26, 2017

    But is it useful to test N memory allocation failures in a row?

    I think it is useful. See my previous post.

    @vstinner
    Copy link
    Member

    2017-06-26 15:34 GMT+02:00 Xavier de Gaye <report@bugs.python.org>:

    python: Objects/call.c:89: _PyObject_FastCallDict: Assertion `!PyErr_Occurred()' failed.
    Aborted (core dumped)

    I will create a new issue when the current issue is closed.

    Oh, I'm curious about that one :-)

    @xdegaye
    Copy link
    Mannequin Author

    xdegaye mannequin commented Jun 30, 2017

    Oh, I'm curious about that one :-)

    This is bpo-30817.

    @xdegaye
    Copy link
    Mannequin Author

    xdegaye mannequin commented Jul 1, 2017

    New changeset 85f6430 by xdegaye in branch 'master':
    bpo-30695: Add set_nomemory(start, stop) to _testcapi (GH-2406)
    85f6430

    @xdegaye
    Copy link
    Mannequin Author

    xdegaye mannequin commented Oct 23, 2017

    Test cases in issues bpo-30697 and bpo-30817, back ported to 3.6, need _testcapi.set_nomemory().

    @xdegaye
    Copy link
    Mannequin Author

    xdegaye mannequin commented Oct 23, 2017

    New changeset aaf6a3d by xdegaye (Miss Islington (bot)) in branch '3.6':
    [3.6] bpo-30695: Add set_nomemory(start, stop) to _testcapi (GH-2406) (bpo-4083)
    aaf6a3d

    @xdegaye xdegaye mannequin closed this as completed Oct 23, 2017
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life tests Tests in the Lib/test dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants