swift-server / swift-aws-lambda-runtime

Swift implementation of AWS Lambda Runtime
Apache License 2.0
1.12k stars 100 forks source link

Avoid warning when enabling strict concurrency check #322

Closed sebsto closed 1 month ago

sebsto commented 2 months ago

Avoid strict concurrency check warnings

Motivation:

My Lambda function is an actor to protect instances variables from race condition.

@main
actor MyLambda: LambdaHandler { ... }

However, when I compile with

swift build -Xswiftc -strict-concurrency=complete

I receive this warning

warning: non-sendable type 'Self.Event' in parameter of the protocol requirement satisfied by actor-isolated instance method 'handle(_:context:)' cannot cross actor boundary
    func handle(_ event: APIGatewayV2Request, context: AWSLambdaRuntimeCore.LambdaContext) async throws -> APIGatewayV2Response {

What I understand from the error message is that Event must be Sendable. But Event is an associated type on LambdaHandler, see https://github.com/swift-server/swift-aws-lambda-runtime/blob/main/Sources/AWSLambdaRuntimeCore/LambdaHandler.swift#L146

In this case, it is an APIGatewayV2Request which correctly implements Sendable but it looks like the compiler can not detect that.

Modifications:

I changed all occurences of associatedtype Event and associatedtype Output in LambdaHandler.swift to add :Sendable. I did not check for Swift >=5.6 because I understand Swift 5.7 is the minimum version to compile the runtime.

Result:

No compiler warnings when using Xswiftc -strict-concurrency=complete

sebsto commented 2 months ago

@tomerd FYI and approval. Let me know if #if swift(>=5.6) checks are required or not

tomerd commented 2 months ago

this seems reasonable to me. @fabianfett thoughts?

ktoso commented 2 months ago

looks sensible to me 👍

sebsto commented 2 months ago

It looks like there are some errors I did not capture locally.

First API change. Is there something I can do to tell the CI system this is indeed an intended API change ?

09:25:45 3 breaking changes detected in AWSLambdaRuntime:
09:25:45   💔 API breakage: extension SimpleLambdaHandler has generic signature change from  to <Self.Event : Swift.Sendable, Self.Output : Swift.Sendable>
09:25:45   💔 API breakage: extension LambdaHandler has generic signature change from  to <Self.Event : Swift.Sendable, Self.Output : Swift.Sendable>
09:25:45   💔 API breakage: extension EventLoopLambdaHandler has generic signature change from  to <Self.Event : Swift.Sendable, Self.Output : Swift.Sendable>
09:25:45 
09:25:45 3 breaking changes detected in AWSLambdaRuntimeCore:
09:25:45   💔 API breakage: protocol SimpleLambdaHandler has generic signature change from  to <Self.Event : Swift.Sendable, Self.Output : Swift.Sendable>
09:25:45   💔 API breakage: protocol LambdaHandler has generic signature change from  to <Self.Event : Swift.Sendable, Self.Output : Swift.Sendable>
09:25:45   💔 API breakage: protocol EventLoopLambdaHandler has generic signature change from  to <Self.Event : Swift.Sendable, Self.Output : Swift.Sendable>

The second set of errors seems to deal with strict concurrency checks. It is not specific to this branch / set of changes. The same errors are generated when building the main branch. I guess I should not care about these right now.

➜  swift-aws-lambda-runtime git:(main) ✗ docker-compose -f docker/docker-compose.yaml -f docker/docker-compose.al2.main.yaml -p swift-aws-lambda-runtime-swift-nightly-prb run --rm test
....
Building for debugging...
/code/Sources/AWSLambdaRuntimeCore/Terminator.swift:74:34: error: reference to captured var 'iterator' in concurrently-executing code; this is an error in the Swift 6 language mode
 72 |                     errors.append(error)
 73 |                 }
 74 |                 return terminate(iterator, errors: errors, promise: promise)
    |                                  `- error: reference to captured var 'iterator' in concurrently-executing code; this is an error in the Swift 6 language mode
 75 |             }
 76 |         }
...

Any idea how to address these ?

fabianfett commented 2 months ago

First API change. Is there something I can do to tell the CI system this is indeed an intended API change ?

This CI step isn't required. It is fine to be red. We use it as a warning signal. The only way to make it not red, is to not make breaking changes. But even then it sometimes has false positives.

sebsto commented 1 month ago

as per discussion with @fabianfett, let's not merge this change. It should be fixed in Swift 6.0