feat: allow async methods to accept &self/&mut self

[wyfo]

Relates to #1632

Draft based on #3608

[codspeed-hq]

CodSpeed Performance Report

Merging #3609 will not alter performance

_{Comparing wyfo:async_receiver (f34c70c) with main (4baf023)}

Summary

✅ 78 untouched benchmarks

[wyfo]

As previously mentioned in #3540 (comment), there is a design decision to be discussed here.
&self/&mut self are not available yet for async methods because of the Send + 'static' requirement. In fact, even if you capture Pyin the future, you cannot holdPyCell/PyRef/PyRefMutacross future suspension points, because they are notSend. It's still possible to hack the requirement by actually capturing Py`, but playing with borrowing manually. But we need to define the borrowing semantic for async method receiver, and there are three possibilities:

• borrow the cell at the coroutine instantiation, and release it when it's dropped
• borrow the cell the first time the coroutine is polled, and release it when it's dropped
• borrow the cell each time the coroutine is polled, and release it at the end of the poll.

In this PR, I've implemented the first one, as illustrated in the related test, because I think it's the simpler to reason about.

Another solution could be to expose Send equivalent to PyRef/PyRefMut, to give more flexibility to users.

[wyfo]

It seems my current implementation has a big MSRV issue, because it only works in 1.74 🤔

I don't find anything in 1.74 changelog that would explain the error observed in the CI job. I will investigate more.

[davidhewitt]

@wyfo I got a bit lost with the recent opening of several PRs, this is the only one not in draft, are you waiting for reviews on any of these (or this one)?

[wyfo]

Indeed, sorry, I should have made it more explicit.
In #3540, I wrote:

So here is how I see the next steps:

• Add the missing documentation, make Coroutine::new private and merge the PR. Also answer the question about async_support branch.
• I will have to retro-engineer my branch to split it in independent PR. It might not be easy for some parts, but that's too bad for me, I should have work in a more structured way instead of writing everything at the same time 😅 I will have to order them to avoid conflicts. This order could be as following
  -name/qualname + not-awaited warning
• #[pyo3(cancel_handle = "param")]
• &self/&mut self
  -#[pyo3(allow_threads)]
• PyFutures
• generic waker with multiple Python runtime support; for this one I have an issue about allowing the user to define a default runtime at the crate level, and I'm asking your help on it (I will open the discussion in the dedicated PR).
• Expose Coroutine constructor; that will mark the achievement of the design.

Tell me if you want to change the order. I will open all the PR at the same time, with each one based on the previous one in the defined order. This way, you will be able to review all the PR independently, as long as they are merged in order.

That's what I've done, but with PR based on a previous one as draft, because they can't be merged before their base PR.
So as long as the final #3613 is not merged, you should always have one "ready to merge" PR to review.
In fact, having multiple PR allows parallel reviews of distinct and quite unrelated features – of course, if you have the time 😅 – as it was the case for #3599 and #3608.

[wyfo]

Ok, now it seems to work in 1.56, so it's ready to review/merge.

[davidhewitt]

Great! Review posted, sorry for being generally slow :)

[davidhewitt]

Is it worth pointing out here that there's a good case for being extra careful with &mut self as a method receiver in a future? I think it's very likely users will accidentally hit borrow errors due to overlapping futures.

We could potentially restrict this to &self on frozen classes? That would help prevent users creating aliasing borrow errors from async code. What do you think about that restriction?

[wyfo]

We could potentially restrict this to &self on frozen classes?

Honestly, I prefer to just add a warning in the documentation. I don't want to prevent careful users to use a mpsc::Receiver in an efficient way.
Maybe you could ping another maintainer to discuss this decision?

[davidhewitt]

Ok let's keep this. I think what I'd prefer to do is make frozen the default and then document what the risks of &mut are if users choose to enable this.

[davidhewitt]

But that is a separate PR.

[davidhewitt]

This looks good to me, thanks 👍

[adamreichold]

there are three possibilities:

* borrow the cell at the coroutine instantiation, and release it when it's dropped

* borrow the cell the first time the coroutine is polled, and release it when it's dropped

* borrow the cell each time the coroutine is polled, and release it at the end of the poll.

In this PR, I've implemented the first one, as illustrated in the related test, because I think it's the simpler to reason about.

I am obviously late to the party, but I think there is a reason besides simplicity for choosing the first option: I think it also matches the behaviour of plain Rust async methods, i.e. writing async fn incr(&mut self) will desugar into something like fn incr<'a>(&'a mut self) -> impl Future<Output=()> + 'a where the borrow lives as long as the future does.

In any case, if I had fast enough to join the fray, I would have asked for the guide entry to expand on this, i.e. to explicitly call out how long the method receiver borrow lasts and that this is natural transfer of the existing semantics of async methods.

[jopemachine]

@wyfo Thanks for your great work.

May I have a question?
I got the below compile error when I tried to specify some argument (num) with the async method.
Is there a way to avoid this error?

future cannot be sent between threads safely
within `pyo3::PyAny`, the trait `Sync` is not implemented for `UnsafeCell<pyo3::ffi::PyObject>`

    #[pymethods]
    impl Counter {
        #[new]
        fn new() -> Self {
            Self(0)
        }
        async fn get(&self) -> usize {
            self.0
        }
        async fn incr(&mut self) -> usize {
            self.0 += 1;
            self.0
        }

        // Error occurs here because of `num`
        async fn incr_num(&mut self, num: usize) -> Result<usize, Error> {
            self.0 += num;
            Ok(self.0)
        }
    }

[davidhewitt]

@jopemachine let's split that out into a new discussion. Probably the "argument extraction" code for num is being included in the future, but that's blocking code so we can potentially rearrange the macro output.