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

Feedback request: Future cancellation #192

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jb-gcx
Copy link
Contributor

@jb-gcx jb-gcx commented Oct 24, 2024

In my project we would greatly benefit from having futures support cancellation requests.

The code in this PR is just a rough outline showing my approach, it is incomplete and untested. The goal is to have cancellation bridged between languages as well (ie: your swift/objc code can cancel a future from C++), but I have not implemented this yet.

The reason why I'm creating an incomplete PR is that I'd like early feedback (@li-feng-sc 🙏 ). Concretely:

  1. Is there interest in (and time for) supporting this?
    • Cancellation support is complex => Review + Test will need time from you
    • If the answer is no, then please let me know soon so I can redirect my efforts to supporting this internally in my project
  2. If there is interest: is it okay with you to build cancellation into djinni::Future, or would you prefer a separate class, for example CancelableFuture? Do you agree with my implementation approach so far?

Examples

Cancellation forwarding:

// Some cancellation-aware method
Future<void> foo();

Future<void> bar() {
    // Will forward cancellation of our future to foos future
    co_await foo();
}

int main() {
    auto future = bar();
    future.getStopSource().request_stop(); // this cancels bars future, which is forwarded to foos future as well
    bar.get(); // throws a CanceledException, assuming foo notices the cancellation before finishing
}

Checking for cancellation

Future<void> worker_thread() {
    Promise<void> promise{};
    auto future = promise.getFuture();

    std::thread{[promise = std::move(promise)] {
        while (!promise.getStopToken().stop_requested()) {
            // do work
        }
    }}.detach();

    return future;
}
void subscribe_to_event(std::function<void()>);
void unsubscribe_from_event();
Future<void> wait_for_event() {
    auto promise = std::make_shared<Promise<void>>();
    inplace_stop_callback callback{promise.getStopToken(), [promise] {
        unsubscribe_from_event();
        promise->setException(CancelledException{});
    }};
    subscribe_to_event([promise] {
        promise->setValue();
    });
    return promise->getFuture();
}
Future<void> coroutine() {
    while (!co_await check_cancelled()) {
        // ...
    }

    // alternatively, to abort immediately when cancelled:
    co_await abort_if_cancelled();
}

Notes

The implementation is based on inplace_stop_token, which is approved for C++26. This allows cancellation with a well defined interface and minimal overhead (no extra heap allocations etc). The token is stored in a futures SharedState.

The overhead could probably be further reduced by replacing the mutex with atomics, but I've decided to go for a simple implementation first

@li-feng-sc
Copy link
Contributor

Is there interest in (and time for) supporting this?
Cancellation support is complex => Review + Test will need time from you
If the answer is no, then please let me know soon so I can redirect my efforts to supporting this internally in my project
If there is interest: is it okay with you to build cancellation into djinni::Future, or would you prefer a separate class, for example CancelableFuture? Do you agree with my implementation approach so far?

I'm currently busy on other Snap projects and only have very limited time to review external djinni PRs. But I can raise this with my colleagues and see if there is someone willing to help drive this.

If we are going to support cancellation, I would rather it to be available across the board and does not require a separate class.

@jb-gcx
Copy link
Contributor Author

jb-gcx commented Oct 28, 2024

I'm currently busy on other Snap projects and only have very limited time to review external djinni PRs. But I can raise this with my colleagues and see if there is someone willing to help drive this.

Sounds good. I would much appreciate if you'd let me know either way - if you don't find someone that's also important to know, so I can focus on rolling our own implementation for my project.

@li-feng-sc
Copy link
Contributor

I'm currently busy on other Snap projects and only have very limited time to review external djinni PRs. But I can raise this with my colleagues and see if there is someone willing to help drive this.

Sounds good. I would much appreciate if you'd let me know either way - if you don't find someone that's also important to know, so I can focus on rolling our own implementation for my project.

I talked to a few people but it seems everyone is busy on higher priority tasks.

@jb-gcx
Copy link
Contributor Author

jb-gcx commented Oct 29, 2024

I talked to a few people but it seems everyone is busy on higher priority tasks.

Okay, thanks for letting me know 👍

I'll redirect my efforts to an internal implementation then. I'll leave it up to you whether to keep this open for future reference or just close it, however you like.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants