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

[core] Add cancellation handling for nextInvocation() #459

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

adam-fowler
Copy link
Member

Allow LambdaChannelHandler.nextInvocation to be cancelled.

Motivation:

If we want to use ServiceLifecycle with the lambda runtime the lambda runtime needs to be cancellable either via a ServiceLifecycle graceful shutdown or via Task cancellation. To avoid bringing in the ServiceLifecycle dependency this PR adds cancellation via Task cancellation handler.

Modifications:

Wrap withCheckedThrowingContinuation call with a withTaskCancellationHandler.
Added LambdaRuntimeClientTests.testCancellation

Result:

You can now cancel the runtime while it is waiting for the next invocation.

I am slightly concerned about this PR though (although I get no Sendability warnings/errors). The process inside the task withTaskCancellationHandler is not running on the event loop (which is why we need the context.eventLoop.execute {}) but I am allowed to set the handler state which I assumed would be isolated to the event loop. Would be good is @fabianfett or someone had a look at this before it was merged.

@sebsto sebsto added the semver/none No version bump required. label Jan 12, 2025
@sebsto sebsto changed the title Add cancellation handling for nextInvocation() [core] Add cancellation handling for nextInvocation() Jan 14, 2025
@sebsto sebsto added this to the 2.0 milestone Jan 16, 2025
@adam-fowler
Copy link
Member Author

This PR does not compile in swift 6 mode and I don't see an easy way around the issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/none No version bump required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants