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

[Draft] Detached tasks #334

Merged
merged 14 commits into from
Aug 23, 2024
Merged

[Draft] Detached tasks #334

merged 14 commits into from
Aug 23, 2024

Conversation

Buratti
Copy link
Contributor

@Buratti Buratti commented Jun 24, 2024

Introduces a system that allows to keep the execution environment running after submitting the response for a synchronous invocation.

Note:

This is a draft PR, only meant to gather feedbacks. This code requires some polishing and testing.
The lines commented with // To discuss: require careful consideration.

Motivation:

It is common for a Lambda function to depend on several other serverless services (SQS, APIGW WebSockets, SNS, EventBridge, X-Ray just to name a few). In many occasions, such services might perform non-critical work like sending a push notification to the user, store metrics or flush a set of spans. In these scenarios, the occurrence of a non-recoverable error usually doesn't mean that the whole invocation should fail and result in a sad 500. At the same time, awaiting for such tasks to finish gives no benefits and drastically increases the latency for the API consumers.

This PR implements a hook in the runtime which allows the developer to dispatch code that can continue its execution after the invocation of /response and before having the environment being frozen by /next. This is a common practice described here.

Modifications:

A new class called DetachedTaskContainer has been added, which follows the design of LambdaTerminator as close as possible. A LambdaContext now owns an instance of DetachedTaskContainer, which can be used by the handler to dispatch asynchrouns non-critical work that can finish after the invocation of /response and before /next.

Result:

It is now possible to return the result of a synchronous invocation (like API Gateway integration, CloudFront origin, Lambda function URL, Cognito Custom Auth, etc), as soon as it is ready, reducing the overall API latency.

func handle(_ buffer: ByteBuffer, context: LambdaContext) -> EventLoopFuture<ByteBuffer?> {
    // EventLoop API
    context.tasks.detached(name: "Example 1") { eventLoop in
        return performSomeWork(on: eventLoop).map { result in
            context.logger.info("\(result) has been provided at \(Date())")
        }
    }
    // Async API
    context.tasks.detached(name: "Example 2") {
        try await Task.sleep(for: .seconds(3))
        context.logger.info("Hello World!")
    }
    let response = encodedAPIGatewayResponse()
    return context.eventLoop.makeSucceededFuture(response)
}

@Buratti Buratti marked this pull request as draft June 24, 2024 16:35
@Buratti
Copy link
Contributor Author

Buratti commented Jun 24, 2024

@sebsto @adam-fowler any feedback is welcome

@sebsto
Copy link
Contributor

sebsto commented Jun 24, 2024

I love the idea. I need more time to look at the implementation
FYI, we're planning to rewrite the runtime for a v2. a rewrite to simplify and better embrace Swift concurrency and service lifecycle. I wonder at which point it is pertinent to include this type of functionality now or wait for the rewrite.
@fabianfett wdyt ?

@Buratti
Copy link
Contributor Author

Buratti commented Jun 24, 2024

That's great! I would love to contribute to a more Swift Concurrency friendly runtime.
About this specific feature, I wouldn't mind seeing in the v1 as well. I have been testing it in our staging environment and the difference in latency is indeed noticeable. Also, there are no breaking changes in this PR.

@sebsto sebsto requested a review from fabianfett June 25, 2024 11:13
@sebsto sebsto self-assigned this Jun 25, 2024
@sebsto sebsto added enhancement New feature or request 🔼 needs-minor-version-bump For PRs that when merged cause a bump of the minor version, ie. 1.x.0 -> 1.(x+1).0 labels Jun 25, 2024
@sebsto
Copy link
Contributor

sebsto commented Jun 25, 2024

@swift-server-bot test this please

Copy link
Member

@fabianfett fabianfett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not in favor of this pr for two reasons:

  1. AWS Lambda does not support background processing in the way this pr tries to add this functionality. AWS Lambda freezes the users execution environment as soon as a request has finished. This means we can not rely on any background processing in Lambda. The correct way to implement this is to wrap the Extensions API, that allows background processing for which users are then charged correctly.

  2. The proposed approach does not integrate nicely with Swift structured concurrency. The correct Swift structured concurrency approach should work as follows:

enum MyBackgroundEvents {
  case myEvent(String)
}

let (stream, continuation) = AsyncStream.makeStream(of: MyBackgroundEvents.self)
let runtime = LambdaRuntime { (request, context) -> String in
  continuation.yield(.myEvent("whatever"))  
  return "Woohoo"
}

await withTaskGroup {
  taskGroup.addTask {
    try await runtime.run()
  }

  taskGroup.addTask {
    for await event in stream {
      // do the actual background processing
    }
  }
}

We intend to build out an API like the one above. However currently we are blocked on this ServiceLifecycle issue.

However even though this is the correct Swift approach, it is very wrong in the Lambda usecase (see 1.). The real thing is to write a wrapper based on the same primitives that correctly implements the Extensions API.

@Buratti
Copy link
Contributor Author

Buratti commented Jun 25, 2024

  1. Actually that's not correct. AWS Lambda freezes the execution environment when /next is invoked. The time elapsed between /response|error and /next is billed normally. The article from AWS Blogs I linked in the first comment explains in details both approaches (this one and the one leveraging the Internal Extensions API).

  2. The proposed design tries to be as much as possible aligned to the way Swift concurrency works.

// Pure Swift
Task.detached {
  try await fireAndForget()
}

// Lambda handler
context.task.detached {
  try await fireAndForget()
}

@fabianfett
Copy link
Member

fabianfett commented Jul 5, 2024

Having thought about this a bit more, I think I have to conclusion that I'm open to this patch in v1. For v2, we should 100% make the concurrency approach structured. A new API proposal will be made shortly.

@Buratti thanks for pointing out that lambda supports background processing.

Sources/AWSLambdaRuntimeCore/DetachedTasks.swift Outdated Show resolved Hide resolved
Comment on lines 167 to 169
extension DetachedTasksContainer: @unchecked Sendable {}
extension DetachedTasksContainer.Storage: @unchecked Sendable {}
extension DetachedTasksContainer.RegistrationKey: Sendable {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We must make sure that all those types are Sendable without using @unchecked. I'm sure we can achieve this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we can, I took inspiration from Terminator.swift for that part, but I'll change it.

Sources/AWSLambdaRuntimeCore/DetachedTasks.swift Outdated Show resolved Hide resolved
Sources/AWSLambdaRuntimeCore/DetachedTasks.swift Outdated Show resolved Hide resolved
Sources/AWSLambdaRuntimeCore/LambdaContext.swift Outdated Show resolved Hide resolved
Sources/AWSLambdaRuntimeCore/DetachedTasks.swift Outdated Show resolved Hide resolved
Sources/AWSLambdaRuntimeCore/DetachedTasks.swift Outdated Show resolved Hide resolved
Sources/AWSLambdaRuntimeCore/DetachedTasks.swift Outdated Show resolved Hide resolved
@fabianfett
Copy link
Member

@Buratti Sorry for not making this explicit. Thanks for pushing on this! Really appreciated. Gave me quite a bit food for thought! Let's try to land this!

@Buratti
Copy link
Contributor Author

Buratti commented Jul 5, 2024

Hi @fabianfett! Thank you for giving this a deeper thought and for the feedbacks, I'll make sure to address them in the next couple of days.
I'm very glad to hear you have reconsidered this feature, and yes, I definitely agree on having a fully structured concurrency version of it for the v2!

// 3. report results to runtime engine
self.runtimeClient.reportResults(logger: logger, invocation: invocation, result: result).peekError { error in
logger.error("could not report results to lambda runtime engine: \(error)")
}
// To discuss:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only point that remains open @fabianfett. When the runtime fails to report a result to AWS Lambda, do we want to wait for the background tasks to complete before stopping the execution of the whole process?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the error case we should await all subtasks.

@sebsto sebsto marked this pull request as ready for review August 1, 2024 15:19
@sebsto
Copy link
Contributor

sebsto commented Aug 1, 2024

Thank you @Buratti for having submitted this change.
As we discussed, we're going to merge it to the main branch while, at the same time, adopting a different and not compatible approach for v2
Can you address the last remaining point ?

@Buratti
Copy link
Contributor Author

Buratti commented Aug 1, 2024

Hi @sebsto! Please forgive me, last few weeks have been very full! I'll try my best to have them ready before tomorrow EOD!

@sebsto
Copy link
Contributor

sebsto commented Aug 5, 2024

@swift-server-bot test this please

@sebsto
Copy link
Contributor

sebsto commented Aug 5, 2024

Thank you for the changes !

Can you add a example code + a section in the README doc - that will allows developers to learn about that feature without having to look at the internals of the code :-)

It looks like the code does not compile on 5.7 - Is this something easy to fix ?

17:37:09 /code/Sources/AWSLambdaRuntimeCore/LambdaRunner.swift:113:10: error: type of expression is ambiguous without more context
17:37:09         .flatMap { context in
17:37:09 ~~~~~~~~~^~~~~~~~~~~~~~~~~~~~

@fabianfett
Copy link
Member

@Buratti Please have a look at #339.

@Buratti
Copy link
Contributor Author

Buratti commented Aug 13, 2024

Hi @sebsto @fabianfett.
I should be able to fix the PR this evening (CEST).

Thanks for sharing the V2 proposal with me, I will read it thoroughly as soon as I finished the changes to this PR.

@fabianfett
Copy link
Member

@swift-server-bot test this please

@Buratti
Copy link
Contributor Author

Buratti commented Aug 16, 2024

Should I run the soundness script?

@sebsto
Copy link
Contributor

sebsto commented Aug 16, 2024

I’m off this week with just my phone and crappy network. I’ll check the submission next week.

But to answer your question, yes the soundness script must run without reporting errors.
the nightly build fail can be ignored.

@sebsto
Copy link
Contributor

sebsto commented Aug 23, 2024

Hello @Buratti ! There is still a minor problem on the soudness script, just a formatting issue on DetachedTasks.
You must ensure you run swift format before committing and that the soundness scripts runs successfully. Also be sure to resync your branch to get the latest pull

@sebsto sebsto merged commit 79fa2c2 into swift-server:main Aug 23, 2024
6 of 8 checks passed
aryan-25 added a commit to aryan-25/swift-aws-lambda-runtime that referenced this pull request Aug 27, 2024
commit ab8166a
Author: Franz Busch <privat@franz-busch.de>
Date:   Mon Aug 26 16:36:07 2024 +0200

    [CI] Add GHA CI and release flow (swift-server#340)

    Co-authored-by: Fabian Fett <fabianfett@apple.com>
    Co-authored-by: Sébastien Stormacq <sebastien.stormacq@gmail.com>
    Co-authored-by: Mahdi Bahrami <github@mahdibm.com>

commit 5ecc24f
Author: Andrea Scuderi <andreascuderi@ymail.com>
Date:   Mon Aug 26 13:00:07 2024 +0200

    Add Breeze to projects.md (swift-server#343)

    authored-by: Andrea Scuderi <andrea.scuderi@ymail.com>

commit 8676c89
Author: Sébastien Stormacq <sebastien.stormacq@gmail.com>
Date:   Mon Aug 26 12:25:41 2024 +0200

    apply swiftformat (swift-server#342)

    * apply swiftformat

    * update dep on Swift Docc to v1.3.0

    * force usage of swift docc plugin 1.3.0

commit 79fa2c2
Author: Alessio Buratti <9006089+Buratti@users.noreply.github.com>
Date:   Fri Aug 23 18:50:22 2024 +0200

    [Draft] Detached tasks (swift-server#334)

    * First prototype

    * Fix build

    * Removes task cancellation

    swift-server#334 (comment)

    * Force user to handle errors

    swift-server#334 (comment)

    * Remove EventLoop API

    swift-server#334 (comment)

    * Make DetachedTaskContainer internal

    swift-server#334 (comment)
    swift-server#334 (comment)

    * Removes @unchecked Sendable

    swift-server#334 (comment)

    * Invoke awaitAll() from async context

    * Fix ambiguous expression type for swift 5.7

    * Fix visibility of detachedBackgroundTask

    * Add swift-doc

    * Add example usage to readme

    * Add tests

    ---------

    Co-authored-by: Sébastien Stormacq <sebastien.stormacq@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request 🔼 needs-minor-version-bump For PRs that when merged cause a bump of the minor version, ie. 1.x.0 -> 1.(x+1).0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants