Skip to content

BufferError with memory.release() #79867

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

Closed
thomaswaldmann mannequin opened this issue Jan 8, 2019 · 15 comments
Closed

BufferError with memory.release() #79867

thomaswaldmann mannequin opened this issue Jan 8, 2019 · 15 comments
Labels
3.7 (EOL) end of life interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error

Comments

@thomaswaldmann
Copy link
Mannequin

thomaswaldmann mannequin commented Jan 8, 2019

BPO 35686
Nosy @skrah, @eryksun, @ThomasWaldmann, @tirkarthi
Files
  • issue35686a.py
  • issue35686b.py
  • issue35686c.py
  • 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 2019-02-02.18:19:24.192>
    created_at = <Date 2019-01-08.17:03:20.643>
    labels = ['interpreter-core', 'invalid', 'type-bug', '3.7']
    title = 'BufferError with memory.release()'
    updated_at = <Date 2019-02-02.18:19:24.169>
    user = 'https://github.com/ThomasWaldmann'

    bugs.python.org fields:

    activity = <Date 2019-02-02.18:19:24.169>
    actor = 'skrah'
    assignee = 'none'
    closed = True
    closed_date = <Date 2019-02-02.18:19:24.192>
    closer = 'skrah'
    components = ['Interpreter Core']
    creation = <Date 2019-01-08.17:03:20.643>
    creator = 'Thomas.Waldmann'
    dependencies = []
    files = ['48031', '48032', '48033']
    hgrepos = []
    issue_num = 35686
    keywords = []
    message_count = 15.0
    messages = ['333236', '333237', '333238', '333239', '333240', '333241', '333242', '333243', '333244', '333247', '333248', '333249', '333268', '334321', '334762']
    nosy_count = 4.0
    nosy_names = ['skrah', 'eryksun', 'Thomas.Waldmann', 'xtreak']
    pr_nums = []
    priority = 'normal'
    resolution = 'not a bug'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue35686'
    versions = ['Python 3.5', 'Python 3.6', 'Python 3.7']

    @thomaswaldmann
    Copy link
    Mannequin Author

    thomaswaldmann mannequin commented Jan 8, 2019

    See there:

    borgbackup/borg#4247

    I did the first changeset after seeing some strange exception popping up which it was handling another exception - which I assumed was related to memoryview.release not being called in the original code.

    So it was clear to me, that we should use the CM there. So I added that (first changeset) and that made the code always fail (see first travis-ci link).

    Then removed the CM again and replaced it with functionally equivalent try/finally (second changeset) - that worked.

    So, the question is whether there is some issue in CPython's memoryview contextmanager code that make it fail in such a strange way.

    @thomaswaldmann thomaswaldmann mannequin added type-crash A hard crash of the interpreter, possibly with a core dump 3.7 (EOL) end of life labels Jan 8, 2019
    @tirkarthi
    Copy link
    Member

    Thanks for the report. Can you please add a simple reproducer in Python with what the test is trying to do without dependencies from the project? perhaps with a sample of relevant files used by the test in Travis.

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Jan 8, 2019

    We use "crash" for segmentation fault, but this appears to be a regular traceback that includes BufferError.

    The BufferError message appears to be from mmapmodule.c.

    @skrah skrah mannequin changed the title memoryview contextmanager causing strange crash BufferError with memory.release() Jan 8, 2019
    @skrah skrah mannequin added type-bug An unexpected behavior, bug, or error and removed type-crash A hard crash of the interpreter, possibly with a core dump labels Jan 8, 2019
    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Jan 8, 2019

    The behavior seems to be correct to me: If there are exports, the memoryview cannot be released.

    The application needs to ensure that release() is not called when
    there are exports left.

    @thomaswaldmann
    Copy link
    Mannequin Author

    thomaswaldmann mannequin commented Jan 8, 2019

    working, but potentially problematic because .release is not always called (e.g. in case of an exception).

    @thomaswaldmann
    Copy link
    Mannequin Author

    thomaswaldmann mannequin commented Jan 8, 2019

    with context manager, does not work.

    @thomaswaldmann
    Copy link
    Mannequin Author

    thomaswaldmann mannequin commented Jan 8, 2019

    with try/finally: works.

    @thomaswaldmann
    Copy link
    Mannequin Author

    thomaswaldmann mannequin commented Jan 8, 2019

    hmm, issue tracker does not make it easy to see which file was uploaded for which comment, so:

    a: original code, works, potentially problematic exception handling (memoryview not always being released)

    b: using the memoryview context manager, fails with BufferError

    c: using try/finally to make sure memoryview is released: works

    @thomaswaldmann
    Copy link
    Mannequin Author

    thomaswaldmann mannequin commented Jan 8, 2019

    I see 2 issues here:

    1. I would expect that the CM approach (b) behaves equivalent to try/finally (c), but it does not and even causes an exception.

    2. The traceback given in the BufferError (in (b)) is misleading, it points to the "print", but should rather point to mmap.__exit__).

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Jan 8, 2019

    Well, the problem in b) is that data[:2] creates a new memoryview,
    so the underlying ManagedBufferObject now has two exports:

    • One from the context manager.

    • The second from the slice.

    So memoryview.__exit__() decrements on export, but the second one
    is hanging.

    This actually works as expected because the ManagedBufferObject
    cannot know that it could also release the slice. That's what I
    meant by saying that it's the application's responsibility to
    release all views that are based on the context manager's view.

    One way of doing so would be this:

    with open(fn, 'rb') as fd:
        with mmap.mmap(fd.fileno(), 0, access=mmap.ACCESS_READ) as mm:
            with memoryview(mm) as data:
                with data[:2] as theslice:
                    print(theslice)

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Jan 8, 2019

    s/on export/one export/

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Jan 8, 2019

    Or, obviously:

    with open(fn, 'rb') as fd:
        with mmap.mmap(fd.fileno(), 0, access=mmap.ACCESS_READ) as mm:
            with memoryview(mm)[:2] as data:
                print(data)

    @eryksun
    Copy link
    Contributor

    eryksun commented Jan 9, 2019

    Or, obviously:

    with open(fn, 'rb') as fd:
    with mmap.mmap(fd.fileno(), 0, access=mmap.ACCESS_READ) as mm:
    with memoryview(mm)[:2] as data:
    print(data)

    Doesn't this rely on the immediate finalization of the unreferenced memoryview instance that creates the slice? It would be nice if memoryview supported alternate constructors memoryview(obj, stop) and memoryview(obj, start, stop[, step]).

    @thomaswaldmann
    Copy link
    Mannequin Author

    thomaswaldmann mannequin commented Jan 24, 2019

    https://github.com/borgbackup/borg/pull/4247/files

    this is the current code i have for this (it is not as simple there as in the minimal code samples i attached here).

    i meanwhile think i can not use a contextmanager there.

    do you think this is as good as it gets for this kind of code?

    if you all think this is expected behaviour for the python contextmanagers and can't be done better, this issue can be closed.

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Feb 2, 2019

    Eryk Sun:

    Yes, the behavior is technically not guaranteed. I'm not sure about
    memoryview(x, start, stop, step) but I'll keep it in mind.

    Thomas Waldmann:

    do you think this is as good as it gets for this kind of code?

    I guess so, there's a lot going on in that code fragment.

    @skrah skrah mannequin added the interpreter-core (Objects, Python, Grammar, and Parser dirs) label Feb 2, 2019
    @skrah skrah mannequin closed this as completed Feb 2, 2019
    @skrah skrah mannequin added the invalid label Feb 2, 2019
    @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 interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants