feat: add `coroutine::CancelHandle` #3599

wyfo · 2023-11-26T11:21:23Z

Relates to #1632

See #3540 (comment) for the reason behind CancelHandle implementation.

codspeed-hq · 2023-11-26T12:09:32Z

CodSpeed Performance Report

Merging #3599 will improve performances by 22.03%

_{Comparing wyfo:coroutine_cancel (aa8ef40) with main (81ad2e8)}

Summary

⚡ 1 improvements
✅ 77 untouched benchmarks

Benchmarks breakdown

	Benchmark	`main`	`wyfo:coroutine_cancel`	Change
⚡	`list_via_downcast`	153.9 ns	126.1 ns	+22.03%

davidhewitt

Thanks, I haven't reviewed the full implementation in detail yet because I think there's a little bit of discussion we could have around the design.

First off, there's two alternatives I can think of and I wonder if they're any good.

The more similar alternative would be to have the cancel handle annotation be on the argument:

    #[pyfunction]
    async fn cancellable_sleep(seconds: f64, #[pyo3(cancel_handle)] mut cancel: CancelHandle) -> usize {
        futures::select! {
            _ = sleep(seconds).fuse() => 42,
            _ = cancel.cancelled().fuse() => 0,
        }
    }

and another alternative could be to have a function which returns a future which only resolves if the current coroutine is cancelled:

    async fn cancellable_sleep(seconds: f64) -> usize {
        futures::select! {
            _ = sleep(seconds).fuse() => 42,
            _ = pyo3::coroutine_is_cancelled().fuse() => 0,
        }
    }

I think this would be possible by putting the throw object into the waker and then this future could retrieve it from the waker.

I quite like this for not needing to pass the cancel: CancelHandle argument, but it does feel a bit "global state"-y, which may have unintended footguns.

There's then another question I have in the back of my mind, which is how this interacts with PyFuture. What happens if that inner Python awaitable gets cancelled? It would be quite nice for this cancel handle to also deal with that case, but I don't think it will in this design. Is there a way we can unify these?

pyo3-macros-backend/src/attributes.rs

wyfo · 2023-11-28T03:44:29Z

I like argument annotation, and I prefer it actually.
I don't like free function for two reasons:

It seems to require the same kind of hack I've already done for PyFuture, but I don't like to have too much of this. What should be the behavior if the function is called outside of coroutine context? panic (like I do for PyFuture)? With CancelHandle parameter, the check is already done.
Ignoring cancellation is not really natural with that way, you have to do a select, but join coroutine_is_cancelled with a pending future in order to have it polled, but not selected. With CancelHandle parameter, you have just to not use it.

There's then another question I have in the back of my mind, which is how this interacts with PyFuture. What happens if that inner Python awaitable gets cancelled? It would be quite nice for this cancel handle to also deal with that case, but I don't think it will in this design. Is there a way we can unify these?

PyFuture can't be selected, it must be awaited alone (it panics otherwise). In this case, the send/throw/close methods delegate to the PyFuture (that's why you cannot select), which handle cancellation itself (and may return an error which can checked and handled).

davidhewitt · 2023-11-28T07:29:01Z

I like argument annotation, and I prefer it actually.

Me too, it's more localised. I think the error message when the annotation is forgotten will be the same "CancelHandle does not implement FromPyObject", so I see no other difference.

It seems to require the same kind of hack I've already done for PyFuture, but I don't like to have too much of this. What should be the behavior if the function is called outside of coroutine context? panic (like I do for PyFuture)?

Would this change in the future with the functionality available on nightly? It might be ok for this future to always just be pending if not in a coroutine context.

Ignoring cancellation is not really natural with that way, you have to do a select, but join coroutine_is_cancelled with a pending future in order to have it polled, but not selected.

Can you please explain in a little bit more detail why this is the case?

PyFuture can't be selected, it must be awaited alone (it panics otherwise). In this case, the send/throw/close methods delegate to the PyFuture (that's why you cannot select), which handle cancellation itself (and may return an error which can checked and handled).

👍 - ok let's worry about PyFuture when we get to that PR.

wyfo · 2023-11-29T10:44:28Z

Would this change in the future with the functionality available on nightly?

Yes, normally.

It might be ok for this future to always just be pending if not in a coroutine context.

I prefer panicking here, because I think it's less error prone.

Can you please explain in a little bit more detail why this is the case?

The default behavior is to reraise the error when throw is called. This behavior is changed if a CancelHandleis registered. So to delay or ignore cancellation, you just have to not use theCancelHandlein the code section you want to "shield. How can you do that with a globalcoroutine_is_cancelled`? Because you have to disable the default behavior before the shielded code section, and check for cancellation after.

adamreichold · 2023-12-02T08:39:56Z

guide/src/async-await.md

+use pyo3::coroutine::CancelHandle;
+
+#[pyfunction]
+async fn cancellable(#[pyo3(cancel_handle)]mut cancel: CancelHandle) {


Space missing after the annotation.

guide/src/async-await.md

adamreichold · 2023-12-02T08:41:48Z

pyo3-macros-backend/src/method.rs

@@ -44,6 +45,7 @@ impl<'a> FnArg<'a> {
                    other => return Err(handle_argument_error(other)),
                };

+                let is_cancel_handle = arg_attrs.cancel_handle.is_some();


Please leave an empty line after this statement.

pyo3-macros-backend/src/method.rs

adamreichold · 2023-12-02T08:47:06Z

src/coroutine.rs

+                self.close();
+                return Err(PyErr::from_value(exc.as_ref(py)));
+            }
+            _ => {}


Maybe write this out as (None, _) just to make it more obvious.

src/coroutine/cancel.rs

wyfo · 2023-12-03T10:37:01Z

Thank you a lot for the review. I've added a last test for CancelHandle::is_cancelled.

davidhewitt

Sorry for not managing to review this until just now. All looks great in my eyes, thanks @adamreichold for being so thorough as always!

I just had one tiny suggestion, which may not be a great idea depending on compiler output...

davidhewitt · 2023-11-30T10:58:12Z

tests/ui/invalid_argument_attributes.rs

+
+#[pyfunction]
+fn cancel_handle_synchronous(#[pyo3(cancel_handle)] _param: String) {}
+


It might be nice to add tests to see what goes on in these cases:

Suggested change

#[pyfunction]

fn cancel_handle_wrong_type(#[pyo3(cancel_handle)] _param: String) {}

#[pyfunction]

fn missing_cancel_handle_attribute(_param: pyo3::coroutine::CancelHandle) {}

(I expect will just be type errors, but how hard will it be for the user to see what they did wrong)

I've added your test cases. I let you judge about the compiler output.

davidhewitt

I think the compiler output is as painful as I expected, but anyway let's move forward with this one. Can always find ways to improve later. 👍

davidhewitt reviewed Nov 27, 2023

View reviewed changes

pyo3-macros-backend/src/attributes.rs Outdated Show resolved Hide resolved

wyfo force-pushed the coroutine_cancel branch 3 times, most recently from c5cb1b5 to 170d1e6 Compare November 29, 2023 22:57

wyfo mentioned this pull request Nov 29, 2023

refactor: drop futures_util dependency #3608

Merged

wyfo force-pushed the coroutine_cancel branch 3 times, most recently from 34e25c5 to d66be8b Compare November 30, 2023 02:55

adamreichold reviewed Dec 2, 2023

View reviewed changes

wyfo force-pushed the coroutine_cancel branch from 7eac478 to aa8ef40 Compare December 2, 2023 19:31

adamreichold approved these changes Dec 3, 2023

View reviewed changes

wyfo force-pushed the coroutine_cancel branch from aa8ef40 to 75eaaff Compare December 3, 2023 10:36

wyfo force-pushed the coroutine_cancel branch from 75eaaff to bc8fc72 Compare December 3, 2023 19:57

davidhewitt reviewed Dec 3, 2023

View reviewed changes

feat: add coroutine::CancelHandle

8a674c2

wyfo force-pushed the coroutine_cancel branch from bc8fc72 to 8a674c2 Compare December 4, 2023 06:47

davidhewitt approved these changes Dec 4, 2023

View reviewed changes

davidhewitt added this pull request to the merge queue Dec 4, 2023

Merged via the queue into PyO3:main with commit e8f852b Dec 4, 2023
35 of 36 checks passed

wyfo mentioned this pull request Dec 6, 2023

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add `coroutine::CancelHandle` #3599

feat: add `coroutine::CancelHandle` #3599

wyfo commented Nov 26, 2023

codspeed-hq bot commented Nov 26, 2023 •

edited

Loading

davidhewitt left a comment

wyfo commented Nov 28, 2023 •

edited

Loading

davidhewitt commented Nov 28, 2023

wyfo commented Nov 29, 2023

adamreichold Dec 2, 2023

adamreichold Dec 2, 2023

adamreichold Dec 2, 2023

wyfo commented Dec 3, 2023

davidhewitt left a comment

davidhewitt Nov 30, 2023

wyfo Dec 4, 2023

davidhewitt left a comment


		#[pyfunction]
		fn cancel_handle_synchronous(#[pyo3(cancel_handle)] _param: String) {}

feat: add coroutine::CancelHandle #3599

feat: add coroutine::CancelHandle #3599

Conversation

wyfo commented Nov 26, 2023

codspeed-hq bot commented Nov 26, 2023 • edited Loading

CodSpeed Performance Report

Merging #3599 will improve performances by 22.03%

Summary

Benchmarks breakdown

davidhewitt left a comment

Choose a reason for hiding this comment

wyfo commented Nov 28, 2023 • edited Loading

davidhewitt commented Nov 28, 2023

wyfo commented Nov 29, 2023

adamreichold Dec 2, 2023

Choose a reason for hiding this comment

adamreichold Dec 2, 2023

Choose a reason for hiding this comment

adamreichold Dec 2, 2023

Choose a reason for hiding this comment

wyfo commented Dec 3, 2023

davidhewitt left a comment

Choose a reason for hiding this comment

davidhewitt Nov 30, 2023

Choose a reason for hiding this comment

wyfo Dec 4, 2023

Choose a reason for hiding this comment

davidhewitt left a comment

Choose a reason for hiding this comment

feat: add `coroutine::CancelHandle` #3599

feat: add `coroutine::CancelHandle` #3599

codspeed-hq bot commented Nov 26, 2023 •

edited

Loading

wyfo commented Nov 28, 2023 •

edited

Loading