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

Docs on Parsing arguments should say something about mem mgmt for formatters returning C strings #68466

Closed
blais mannequin opened this issue May 24, 2015 · 12 comments
Closed
Labels
docs Documentation in the Doc dir

Comments

@blais
Copy link
Mannequin

blais mannequin commented May 24, 2015

BPO 24278
Nosy @asvetlov, @vadmium
Files
  • buffer-convert.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 2016-08-04.02:53:17.021>
    created_at = <Date 2015-05-24.15:27:38.376>
    labels = ['docs']
    title = 'Docs on Parsing arguments should say something about mem mgmt for formatters returning C strings'
    updated_at = <Date 2016-08-07.04:18:45.671>
    user = 'https://bugs.python.org/blais'

    bugs.python.org fields:

    activity = <Date 2016-08-07.04:18:45.671>
    actor = 'blais'
    assignee = 'docs@python'
    closed = True
    closed_date = <Date 2016-08-04.02:53:17.021>
    closer = 'martin.panter'
    components = ['Documentation']
    creation = <Date 2015-05-24.15:27:38.376>
    creator = 'blais'
    dependencies = []
    files = ['39504']
    hgrepos = []
    issue_num = 24278
    keywords = ['patch']
    message_count = 12.0
    messages = ['243987', '243989', '243990', '244006', '244032', '244033', '244083', '244091', '244984', '271875', '271935', '272111']
    nosy_count = 5.0
    nosy_names = ['blais', 'asvetlov', 'docs@python', 'python-dev', 'martin.panter']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue24278'
    versions = ['Python 3.5', 'Python 3.6']

    @blais
    Copy link
    Mannequin Author

    blais mannequin commented May 24, 2015

    Functions that parse arguments like PyArg_ParseTupleAndKeywords() have several formatters that fill in C strings, e.g. "s".

    In the C API doc:
    https://docs.python.org/3.5/c-api/arg.html#c.PyArg_ParseTupleAndKeywords

    There should be an explicit mention of whether this memory needs to be free (in this case: No) and if not, how this memory is managed (in this case: This refers to a buffer managed by the string object itself). Because of questions of encoding, it raises questions where this memory lies, and what its lifetime is (in this case: That of the owning string object from the caller).

    This deserves an explicit mention, even if brief.

    @blais blais mannequin assigned docspython May 24, 2015
    @blais blais mannequin added the docs Documentation in the Doc dir label May 24, 2015
    @asvetlov
    Copy link
    Contributor

    Would you propose a patch for docs?

    @blais
    Copy link
    Mannequin Author

    blais mannequin commented May 24, 2015

    I don't think I'm the right person to propose a patch for this; I'm just guessing how it works so far, I haven't had time to look into the source code in detail, I'm sure there's a lot more context to it.

    @vadmium
    Copy link
    Member

    vadmium commented May 24, 2015

    At the top of the list <https://docs.python.org/3.5/c-api/arg.html#strings-and-buffers\>, it says which cases have to be freed or not, and also mentions releasing buffers. Are you proposing to add this information to each entry in the list as well?

    Or just mention how the pointer lifetime relates to the Python object lifetime at the top? I think you will find most of the cases are the same, except for the ones that explicitly allocate extra memory.

    @blais
    Copy link
    Mannequin Author

    blais mannequin commented May 25, 2015

    Adding information that tells developers where the memory for those returned areas is stored and as you mention, its lifetime guarantees w.r.t. to the Python object, would go a long way towards making this more clear. The questions that immediately came to my mind were:

    • Is this memory attached to the object?

    • What if there is a conversion... is it still attached to the object? The converter for "s" says "Unicode objects are converted to C strings using 'utf-8' encoding." Where is the output of this conversion stored? Does it have the same lifetime as its PyObject as well or does it use a cache of recent conversions (e.g. like re/struct), or just static storage? And if so, is it thread-safe?

    I can find all these answers by looking at the source code for C/Python, or I can _guess_ that extra data is attached to some sort of 'extra' field in a PyObject (which would be a sensible thing to do), but my point is that an API user shouldn't have to dig in the source or have to guess for such important concerns. I think we should be a bit more transparent in the docs.

    @blais
    Copy link
    Mannequin Author

    blais mannequin commented May 25, 2015

    Oh, and yes, just adding this info at the top would be fine IMO. It shouldn't have to be repeated.

    @vadmium
    Copy link
    Member

    vadmium commented May 26, 2015

    This is my understanding of where the buffers would be managed and what governs their lifetimes, though I haven’t analyzed the code to verify:

    s, z: str -> UTF-8 cache -> pointer for object lifetime
    s*, z*: str -> UTF-8 cache -> buffer; bytes-like -> buffer; until release
    y*: bytes-like -> buffer until release
    S: bytes borrowed object
    Y: bytearray borrowed object
    u, u#, Z, Z#: str -> Py_UNICODE cache -> pointer for object lifetime
    U: str borrowed object
    w*: writable bytes-like -> buffer until release
    es, es#: str -> encoded -> pointer until free (or pre-allocated for es#)
    et, et#: str -> encoded -> pointer; bytes, bytearray -> pointer; until free (or pre-allocated for et#)

    One open question that has worried me a bit is the “s#”, “z#”, “y”, “y#” codes, which are documented as requiring immutable bytes-like objects, but do not return a buffer structure. I guess this is designed for objects like bytes(), where the pointer would remain valid for the object’s lifetime, even if it has been released according to the buffer protocol. But how is this guaranteed for arbitrary buffer objects? Some undocumented requirement of the buffer’s “readonly” flag perhaps?

    So I propose to add:

    • Lifetime of all Py_buffer return values is until PyBuffer_Release() is called
    • Unless otherwise documented, for conversions that return pointers to buffers, the buffer is managed or cached inside the Python object, and the lifetime of that buffer is the same as the whole Python object
    • Unconverted Python objects are borrowed references
    • For the four immutable bytes-like conversions I mentioned that return pointers, the buffer management and lifetime is not documented (unless somebody comes up with a better explanation)

    @vadmium
    Copy link
    Member

    vadmium commented May 26, 2015

    Here is a patch.

    Looking at the code, s#, z#, y and y# are the conversions that call convertbuffer(). These require that the object does not have a PyBufferProcs.bf_releasebuffer() method, which guarantees that the buffer’s lifetime is the same as the underlying object.

    In the patch I also removed a contradictory notice about nulls with the “u” conversion.

    @blais
    Copy link
    Mannequin Author

    blais mannequin commented Jun 8, 2015

    LGTM! Thanks for making the change.

    @vadmium
    Copy link
    Member

    vadmium commented Aug 3, 2016

    Will commit this soon, apart from dropping “such as bytearray” for s#, which I don’t remember the reasoning.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Aug 4, 2016

    New changeset ad7da726bea6 by Martin Panter in branch '3.5':
    Issue bpo-24278: Explain how argument parsing output buffers are managed
    https://hg.python.org/cpython/rev/ad7da726bea6

    New changeset 49c318c6294e by Martin Panter in branch 'default':
    Issue bpo-24278: Merge argument parsing docs from 3.5
    https://hg.python.org/cpython/rev/49c318c6294e

    @vadmium vadmium closed this as completed Aug 4, 2016
    @blais
    Copy link
    Mannequin Author

    blais mannequin commented Aug 7, 2016

    Thank you Martin!

    @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
    docs Documentation in the Doc dir
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants