classification
Title: PEP-3118: remove obsolete write-locks
Type: resource usage Stage: resolved
Components: Interpreter Core Versions: Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: kelleynnn, ncoghlan, pitrou, python-dev, serhiy.storchaka, skrah
Priority: normal Keywords: needs review, patch

Created on 2012-03-05 18:32 by skrah, last changed 2015-02-04 13:51 by skrah. This issue is now closed.

Files
File name Uploaded Description Edit
bytearray_getbuffer.diff skrah, 2012-03-05 18:32 review
issue14203-2.diff skrah, 2015-02-03 15:14 review
issue14203-3.diff skrah, 2015-02-03 20:17 review
Messages (15)
msg154969 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2012-03-05 18:32
bytearray_getbuffer() checks for view==NULL. But this has never been
allowed:

PEP: "The second argument is the address to a bufferinfo structure.
      Both arguments must never be NULL."

DOCS (3.2): "view must point to an existing Py_buffer structure
             allocated by the caller".


A quick grep through the source tree shows no instances where
the middle argument of either PyObject_GetBuffer of bf_getbuffer
is NULL or 0.


Patch attached, all tests pass. I wouldn't be comfortable to
commit it without review though (it's just too strange).


BTW, the next conditional in bytearray_getbuffer ...

    if (ret >= 0) {
        obj->ob_exports++;
    }

is also superfluous, since PyBuffer_FillInfo() cannot fail
if readonly==0.
msg154971 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2012-03-05 18:56
array_buffer_getbuf does a similar thing:

if (view==NULL) goto finish;

 finish:
    self->ob_exports++;   // ???
    return 0


Where does this view==NULL thing come from?
msg154972 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2012-03-05 19:25
I seems to be a feature to get a "lock" on an exporter
without the exporter filling in a buffer. It was there
from the beginning:

http://svn.python.org/view/python/branches/release30-maint/Objects/bytearrayobject.c?r1=56849&r2=57181


Last use that I can see is here:

http://hg.python.org/cpython/file/df3b2b5db900/Modules/posixmodule.c#l561
msg219995 - (view) Author: Kelley Nielsen (kelleynnn) Date: 2014-06-07 22:51
I have verified that this feature is unused in the source tree; in fact, there are no internal calls to bytearray_getbuffer() at all. The only thing bytearray_getbuffer() does with its second arg is pass it to PyBuffer_FillInfo(), which immediately checks it and passes 0 up the call stack if it is NULL. (A comment in PyBuffer_FillInfo() asks why -1 is not passed up instead; it's probably to distinguish this feature from the error condition handled in the immediately following conditional block.)

There are potentially other issues stemming from this legacy feature in bytearray_getbuffer(), PyBuffer_FillInfo(), and elsewhere. The maintainers may see fit to open tickets on these issues as well.

There's more relevant commentary on the feature here: http://comments.gmane.org/gmane.comp.python.devel/130521
msg235346 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2015-02-03 15:14
New patch with tests.
msg235348 - (view) Author: Roundup Robot (python-dev) Date: 2015-02-03 15:57
New changeset e8fe32d43c96 by Stefan Krah in branch 'default':
Issue #14203: Remove obsolete support for view==NULL in PyBuffer_FillInfo()
https://hg.python.org/cpython/rev/e8fe32d43c96
msg235352 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2015-02-03 16:35
bytesiobuf_getbuffer() also still has this obsolete feature, so
BufferError should be raised if view==NULL.

I'm unsure if this plays well with the new SHARED_BUF(b) thing.

Should the exception be raised before or after?
msg235354 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-02-03 17:05
I think before. It doesn't harm, but it doesn't make much sense to unshare the buffer if its address is not used.
msg235355 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-02-03 17:11
E.g. the buffer should be unshared before incrementing b->exports, but if an exception is raised instead, there is no need to unshare the buffer before raising.
msg235358 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2015-02-03 20:17
Ok, thanks!  issue14203-3.diff should be correct, then.
msg235359 - (view) Author: Roundup Robot (python-dev) Date: 2015-02-03 20:43
New changeset 0d5095a2422f by Stefan Krah in branch 'default':
Issue #14203: Remove obsolete support for view==NULL in bytesiobuf_getbuffer()
https://hg.python.org/cpython/rev/0d5095a2422f
msg235360 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-02-03 20:57
Compiling failed on Windows: http://buildbot.python.org/all/builders/AMD64%20Windows7%20SP1%203.x/builds/5650
msg235361 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2015-02-03 21:02
Argh, the extern _PyBytesIOBuffer_Type hack.  I knew it would cause some
trouble.

Any ideas how to test bytesiobuf_getbuffer() with a NULL view in
a more dignified way?
msg235363 - (view) Author: Roundup Robot (python-dev) Date: 2015-02-03 21:27
New changeset 17a8c5f8ca48 by Stefan Krah in branch 'default':
Issue #14203:  Temporary fix for the compile failure on Windows.
https://hg.python.org/cpython/rev/17a8c5f8ca48
msg235386 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2015-02-04 13:51
I think it's sufficient to test bytesiobuf_getbuffer() on
Linux and FreeBSD.  The test just checks that the exception
is raised.
History
Date User Action Args
2015-02-04 13:51:46skrahsetstatus: open -> closed
resolution: fixed
messages: + msg235386

stage: patch review -> resolved
2015-02-03 21:27:57python-devsetmessages: + msg235363
2015-02-03 21:02:46skrahsetmessages: + msg235361
2015-02-03 20:57:28serhiy.storchakasetmessages: + msg235360
2015-02-03 20:43:54python-devsetmessages: + msg235359
2015-02-03 20:17:27skrahsetfiles: + issue14203-3.diff

messages: + msg235358
2015-02-03 17:11:20serhiy.storchakasetmessages: + msg235355
2015-02-03 17:05:45serhiy.storchakasetmessages: + msg235354
2015-02-03 16:35:24skrahsetnosy: + serhiy.storchaka

messages: + msg235352
title: bytearray_getbuffer: unnecessary code -> PEP-3118: remove obsolete write-locks
2015-02-03 15:57:59python-devsetnosy: + python-dev
messages: + msg235348
2015-02-03 15:14:14skrahsetfiles: + issue14203-2.diff

messages: + msg235346
2014-06-07 22:51:54kelleynnnsetnosy: + kelleynnn

messages: + msg219995
versions: + Python 3.5, - Python 3.3
2012-03-06 16:39:42skrahlinkissue13860 superseder
2012-03-06 16:39:09skrahunlinkissue13860 dependencies
2012-03-05 19:25:31skrahsetmessages: + msg154972
2012-03-05 19:24:52skrahlinkissue13860 dependencies
2012-03-05 18:56:14skrahsetmessages: + msg154971
2012-03-05 18:32:48skrahcreate