-
-
Notifications
You must be signed in to change notification settings - Fork 30.6k
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 context manager protocol to memoryviews #53966
Comments
This adds the context manager protocol to memoryview objects, as proposed on python-dev. Once the |
Guido expressed skepticism at the idea: |
I closed 9789 as a duplicate of this. Bringing the details from that issue over here: This may cause problems for mutable objects that are locked while PEP-3118 buffer references remain unreleased (e.g. in 3.2, io.BytesIO supports getbuffer() for direct access to the underlying memory, but disallows resizing until the associated memoryview goes away). This isn't too bad in CPython due to explicit refcounting, but may be an issue when using other implementations since the time between release of the last reference and actual garbage collection is indeterminate. For example, the new test_getbuffer in the BytesIOMixin class in the test suite can't rely on "del buf" promptly releasing the underlying PEP-3118 buffer, so it is forced to also invoke a full GC collection cycle in order to be portable to other implementations. So there are two separate questions here:
Guido was -0 on the idea of supporting the context management protocol, but the rationale presented to him at the time was lacking the key concept of behavioural changes in the object owning the buffer based on whether or not there were any outstanding buffer references. |
Given this explanation, of course I am +1 on an explicit release() method. But I'm still skeptical that a context manager adds much (not sure if that counts as -0 or +0 :-). I suppose after release() is called all accesses through the memoryview object should be invalid, right? Is this not covered by PEP-3118 at all? |
Ok, release() is probably enough.
Indeed. The patch tests for that (it uses "with" to release the
The PEP says “this memory view object holds on to the memory of base |
As with a few(!) other things in relation to this PEP, the primary +1 on adding release() (obviously), and +1 on direct support for |
Here is a new patch adding the release() method as well. |
On an eyeball review, the structure of do_release seems a little questionable to me. I'd be happier if view.obj and view.buf were copied to C locals and then set to NULL at the start of the function before any real work is done. I believe the buffer release process can trigger __del__ methods (and possibly other Python code), which may currently see the memoryview in a slightly inconsistent state when reference cycles are involved. Setting these two references to NULL early should keep that from happening (the idea of using C locals to avoid this is the same concept as is employed by the Py_CLEAR macro) For the test code, I suggest including:
(For the properties, perhaps using a list of strings, a loop and getattr rather than writing each one out separately) _check_released should perhaps also take a second argument that is used for the last two NotEqual checks. That is: def _check_released(self, m, tp):
...
self.assertNotEqual(m, memoryview(tp()))
self.assertNotEqual(m, tp()) And then pass in tp as well as m for the respective tests. There's also a minor typo in a comment in the tests: "called at second" should be "called a second". Other than that, looks good to me. |
One other question: should IS_RELEASED use "||" rather than "&&"? Is there any case where either of those pointers can be NULL and we still want to continue on rather than bailing out with a ValueError? |
You can't do that, since PyBuffer_Release() needs the information (at (note, this code is simply factored out from tp_dealloc)
Ok. I was bit too lazy and didn't include all the API.
It's more clever but less practical if there's a failure: you can't know
Hmm, why not.
Ah, thanks.
Well, view.obj can be NULL in case the buffer has been filled by hand by I could simply test for view.buf. I was thinking that maybe NULL could |
Sounds good - I'd say just commit whenever you're happy with it then. |
Committed in r84649. Thanks for the comments! |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: