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

Cannot cancel an IAsyncEnumerable #8958

Open
pablo-salad opened this issue Apr 24, 2024 · 3 comments · May be fixed by #9127
Open

Cannot cancel an IAsyncEnumerable #8958

pablo-salad opened this issue Apr 24, 2024 · 3 comments · May be fixed by #9127
Milestone

Comments

@pablo-salad
Copy link

An unexpected behavior has been observed while working with IAsyncEnumerable and we try to cancel it.
A method returning an IAsyncEnumerable, which internally fetches data from the Channel, encounters no issues to be cancelled when there is data in it.
However, if the Channel is empty and the cancellation is requested, there is no execution of code block in the try/finally within the grain.

There is a demo repo to explain this problem in detail.

You'll find three branches detailing various attempts to resolve this issue during our testing phase:

  • main: The channel has data and the cancellation works as expected.

image

  • empty_iasyncenumerable: The channel is empty and the cancellation is called but it didn't cancel the grain code.

image

  • graincancellationtoken: We tried to pass a GrainCancellationToken to the GetCount method and initiate its cancellation upon request from another CancellationToken but unfortunately, the cancellation process did not succeed. Same behavior as in empty_iasyncenumerable.

We would greatly appreciate any assistance on this matter.
Thank you!

@ReubenBond
Copy link
Member

Thank you for reporting. This is a good point. There needs to be a better way to accomplish this. I'll mark this as a feature enhancement for now. Ideally, we have a better way to flow cancellation along grain call chains in general. GrainCancellationToken is not ideal. Ideally, we would support CancellationToken itself, natively.

@ReubenBond ReubenBond added this to the Backlog milestone Apr 29, 2024
@koenbeuk
Copy link
Contributor

koenbeuk commented May 1, 2024

Ideally, we would support CancellationToken itself, natively.

This is important to us, do you have a strategy for this in mind and if so, would you elaborate? If possible we'd like to contribute this feature.

@ReubenBond
Copy link
Member

This is important to us, do you have a strategy for this in mind and if so, would you elaborate? If possible we'd like to contribute this feature.

The feature needs design work - scenarios/requirements could help to drive the design. Lifetime/scope management for the cancellation is the biggest issue. Eg, do we support cancelling a captured CancellationToken after the sending call has completed? Some rough ideas:

  • CodeGen would detect a CancellationToken parameter passed as a method parameter and implement an optional "cancellable invokable" interface for the method, allowing the invocation to be canceled.
  • At-most one CancellationToken parameter is supported per method, at the parameter level (not nested in other objects).
  • For the duration of the method invocation on the client side, we would need to Register for cancellation notifications.
    • If the cancellation token is captured and sent elsewhere, that is invalid: we will either implicitly cancel the token when the method invocation completes on the grain, or we will leave it dangling (never canceled). I prefer the first approach (eagerly cancel). It is similar to the behavior of cancellation in Kotlin's structured concurrency.
    • When the token is canceled, we send a notification to the target grain to signal cancellation via a grain extension. That grain extension is implemented by ActivationData (the grain context) and when it receives the call, it finds the relevant message and cancels its "cancellable invokable".

@koenbeuk koenbeuk linked a pull request Aug 27, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants