Support passing name and doc to new_closure.

[dalcde]

No description provided.

[dalcde]

This is now done On 15 October 2022 4:09:47 pm HKT, Adam Reichold ***@***.***> wrote: ***@***.*** commented on this pull request.

…

> /// py_run!(py, add_one, "assert add_one(42) == 43"); /// }); /// ``` - pub fn new_closure<F, R>(f: F, py: Python<'_>) -> PyResult<&PyCFunction> + pub fn new_closure<'a, F, R>( I think it is also customary to move closure arguments to the end as this improves readability of the call sites. -- Reply to this email directly or view it on GitHub: #2686 (comment) You are receiving this because you authored the thread. Message ID: ***@***.***>

[mejrs]

What about this signature?

pub fn new_closure<'a, F, R>(
        py: Python<'a>,
        name: Option<&'static str,
        doc: Option<&'static str>,
        f: F,
    ) -> PyResult<&'a PyCFunction>

iirc both name and doc can be null (?) and I think I prefer using None rather than "" if I don't care about doc

[mejrs]

I'm not sure if this PR caused this or not, but something like this

use pyo3::prelude::*;
use pyo3::types::*;

fn main() {
    Python::with_gil(|_py| loop {
        let pool = unsafe { _py.new_pool() };
        let py = pool.python();
        let add_one = |args: &PyTuple, _kwargs: Option<&PyDict>| -> PyResult<_> {
            let i = args.extract::<(i64,)>()?.0;
            Ok(i + 1)
        };
        let closure_py = PyCFunction::new_closure(py, "blah", "blah", add_one).unwrap();
    });
}

leaks memory at a rate of 400MB/s. I'm not familiar with closures and capsules, but that doesn't sound right.

[dalcde]

We need to leak memory to convert a &'static str to a &'static cstr. If your name/doc is already null terminated I think we don't leak. This is also the case for normal functions except we can't create them in a loop. I believe this also happens before the PR.

[mejrs]

It leaks on pyo3 version 0.17 too 🤔

[dalcde]

Also I don't think doc/name can be null. A PyMethodDef with null doc and name is used to mark the end of a PyMethodDef array.

[birkenfeld]

If I read the CPython code correctly, at least name should not be NULL. But of course it is easy to unwrap_or("\0").

The leak is easy to fix in 0.17 since we control the literals and can null-terminate them. For after this patch there are several possibilities:

• require &'static CStr (not easy to get without a helper crate, and breaks API consistency)
• require null-terminated string literals and return Err otherwise (also breaks API consistency)
• keep our own static storage of &'static CStrs

[davidhewitt]

Yes, the general use of leaking in current PyO3 is not ideal and it's definitely a bug that each call to new_closure currently leaks.

It's been on the back of my mind to tidy up and I've personally wanted to go down the &'static CStr route but been waiting for an MSRV bump, because &'static CStr can't be const-constructed (i.e. without leaking until CStr::from_bytes_with_nul_unchecked became const in 1.59).

Maybe we should explore a solution like https://crates.io/crates/const-cstr ?

[dalcde]

I think there are two use cases of closures that have different requirements. If the goal is to define a module function but you find it more convenient to define it via a closure for whatever reason, you don't really care about leaks too much because it's going to leave it around forever. The other use case is a short-lived function to feed into other python functions that require a function. In this case you probably don't care about name and doc. Given this, it seems reasonable to have a default new_closure that does not take name/doc and does not leak, plus a version that takes name/doc and leaks if the user doesn't null-terminate.

[adamreichold]

I think there are two use cases of closures that have different requirements. If the goal is to define a module function but you find it more convenient to define it via a closure for whatever reason, you don't really care about leaks too much because it's going to leave it around forever. The other use case is a short-lived function to feed into other python functions that require a function. In this case you probably don't care about name and doc. Given this, it seems reasonable to have a default new_closure that does not take name/doc and does not leak, plus a version that takes name/doc and leaks if the user doesn't null-terminate.

Couldn't the signature proposed in #2686 (comment) fulfil this as well without having separate entry points?

[mejrs]

Let me see what I can come up with. 🙂

[dalcde]

Sure. Two functions would preserve backwards compatibility though and we can make one redirect to the other

[dalcde]

@mejrs Do you think this is blocked on #2690? Or can they proceed independently?

(My understanding of the codebase is that if we use null-terminated static strings in PyMethodDef::cfunction_with_keywords, then it already does not leak. I suppose #2690 prepares for the case of user-supplied non-null-terminated strings?)

[mejrs]

@mejrs Do you think this is blocked on #2690? Or can they proceed independently?

I'll need to do some more thinking on that PR, so I'm happy to merge this one first.

(My understanding of the codebase is that if we use null-terminated static strings in PyMethodDef::cfunction_with_keywords, then it already does not leak. I suppose #2690 prepares for the case of user-supplied non-null-terminated strings?)

That's true, but there is also a boxed PyMethodDef that is getting leaked. I don't mind that for this PR though.

I'd still like the api as #2686 (comment) . It can default to a dummy name/doc if None.

[dalcde]

I've updated the pull request to accept optional name/doc.

[dalcde]

Is this still blocked on anything? Or are we waiting for the next breaking release?

[mejrs]

Is this still blocked on anything? Or are we waiting for the next breaking release?

I just hadn't gotten around to it. Thanks ❤️

[dalcde]

Thanks!