classification
Title: BufferError with memory.release()
Type: behavior Stage: resolved
Components: Interpreter Core Versions: Python 3.7, Python 3.6, Python 3.5
process
Status: closed Resolution: not a bug
Dependencies: Superseder:
Assigned To: Nosy List: Thomas.Waldmann, eryksun, skrah, xtreak
Priority: normal Keywords:

Created on 2019-01-08 17:03 by Thomas.Waldmann, last changed 2019-02-02 18:19 by skrah. This issue is now closed.

Files
File name Uploaded Description Edit
issue35686a.py Thomas.Waldmann, 2019-01-08 18:12
issue35686b.py Thomas.Waldmann, 2019-01-08 18:13
issue35686c.py Thomas.Waldmann, 2019-01-08 18:13
Messages (15)
msg333236 - (view) Author: Thomas Waldmann (Thomas.Waldmann) Date: 2019-01-08 17:03
See there:

https://github.com/borgbackup/borg/pull/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.
msg333237 - (view) Author: Karthikeyan Singaravelan (xtreak) * (Python committer) Date: 2019-01-08 17:20
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.
msg333238 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2019-01-08 18:08
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.
msg333239 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2019-01-08 18:12
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.
msg333240 - (view) Author: Thomas Waldmann (Thomas.Waldmann) Date: 2019-01-08 18:12
working, but potentially problematic because .release is not always called (e.g. in case of an exception).
msg333241 - (view) Author: Thomas Waldmann (Thomas.Waldmann) Date: 2019-01-08 18:13
with context manager, does not work.
msg333242 - (view) Author: Thomas Waldmann (Thomas.Waldmann) Date: 2019-01-08 18:13
with try/finally: works.
msg333243 - (view) Author: Thomas Waldmann (Thomas.Waldmann) Date: 2019-01-08 18:23
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
msg333244 - (view) Author: Thomas Waldmann (Thomas.Waldmann) Date: 2019-01-08 18:28
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__).
msg333247 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2019-01-08 19:35
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)
msg333248 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2019-01-08 19:44
s/on export/one export/
msg333249 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2019-01-08 19:47
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)
msg333268 - (view) Author: Eryk Sun (eryksun) * (Python triager) Date: 2019-01-09 00:27
> 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]).
msg334321 - (view) Author: Thomas Waldmann (Thomas.Waldmann) Date: 2019-01-24 20:23
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.
msg334762 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2019-02-02 18:19
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.
History
Date User Action Args
2019-02-02 18:19:24skrahsetstatus: open -> closed
messages: + msg334762

components: + Interpreter Core
resolution: not a bug
stage: resolved
2019-01-24 20:23:30Thomas.Waldmannsetmessages: + msg334321
2019-01-09 00:27:57eryksunsetnosy: + eryksun
messages: + msg333268
2019-01-08 19:47:53skrahsetmessages: + msg333249
2019-01-08 19:44:45skrahsetmessages: + msg333248
2019-01-08 19:35:30skrahsetmessages: + msg333247
2019-01-08 18:28:40Thomas.Waldmannsetmessages: + msg333244
2019-01-08 18:23:20Thomas.Waldmannsetmessages: + msg333243
2019-01-08 18:13:52Thomas.Waldmannsetfiles: + issue35686c.py

messages: + msg333242
2019-01-08 18:13:18Thomas.Waldmannsetfiles: + issue35686b.py

messages: + msg333241
2019-01-08 18:12:34Thomas.Waldmannsetfiles: + issue35686a.py

messages: + msg333240
2019-01-08 18:12:17skrahsetmessages: + msg333239
2019-01-08 18:08:10skrahsettype: crash -> behavior
messages: + msg333238
title: memoryview contextmanager causing strange crash -> BufferError with memory.release()
2019-01-08 17:46:19serhiy.storchakasetnosy: + skrah
2019-01-08 17:20:00xtreaksetnosy: + xtreak
messages: + msg333237
2019-01-08 17:03:20Thomas.Waldmanncreate