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

adoption of sendable #252

Merged
merged 5 commits into from
Apr 14, 2022
Merged

adoption of sendable #252

merged 5 commits into from
Apr 14, 2022

Conversation

tomerd
Copy link
Contributor

@tomerd tomerd commented Mar 18, 2022

motivation: adopt to sendable requirments in swift 5.6

changes:

  • define sendable shims for protocols and structs that may be used in async context
  • adjust tests
  • add a test to make sure no warning are emittted

@tomerd tomerd requested a review from fabianfett March 18, 2022 02:58
@tomerd tomerd force-pushed the sendable branch 3 times, most recently from 2b236e4 to 17051a1 Compare March 18, 2022 03:20
@tomerd tomerd added this to the 1.0.0-alpha.1 milestone Mar 18, 2022
@tomerd tomerd added the 🔼 needs-minor-version-bump For PRs that when merged cause a bump of the minor version, ie. 1.x.0 -> 1.(x+1).0 label Mar 18, 2022
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 think the really important type is LambdaRuntime, which we need to make Sendable.

Sources/AWSLambdaRuntimeCore/LambdaHandler.swift Outdated Show resolved Hide resolved
Sources/AWSLambdaRuntimeCore/LambdaHandler.swift Outdated Show resolved Hide resolved
Sources/AWSLambdaRuntimeCore/LambdaHandler.swift Outdated Show resolved Hide resolved
@tomerd tomerd force-pushed the sendable branch 2 times, most recently from c80f7c7 to 28d613b Compare April 13, 2022 01:34
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.

Great progress! I think we must not forget to mark LambdaRuntime and LambdaTerminator also as Sendable.

Sources/AWSLambdaRuntimeCore/LambdaHandler.swift Outdated Show resolved Hide resolved
Sources/AWSLambdaRuntimeCore/LambdaHandler.swift Outdated Show resolved Hide resolved
@tomerd
Copy link
Contributor Author

tomerd commented Apr 13, 2022

I think we must not forget to mark LambdaRuntime and LambdaTerminator also as Sendable.

yes, I will merge the terminator PR first than rebase and update

motivation: adopt to sendable requirments in swift 5.6

changes:
* define sendable shims for protocols and structs that may be used in async context
* adjust tests
* add a test to make sure no warning are emittted
@tomerd tomerd changed the title [wip] adoption of sendable adoption of sendable Apr 13, 2022
@@ -137,3 +137,10 @@ extension LambdaTerminator {
let underlying: [Error]
}
}

// Ideally this would not be @unchecked Sendable, but Sendable checks do not understand locks
// We can transition this to an actor once we drop support for older Swift versions
Copy link
Member

Choose a reason for hiding this comment

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

We can transition this to an actor once we have custom executors :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that too

@tomerd
Copy link
Contributor Author

tomerd commented Apr 13, 2022

I think we must not forget to mark LambdaRuntime and LambdaTerminator also as Sendable.

done for LambdaTerminator. why would we want LambdaRuntime to be Sendable? it is not used directly by the users atm afaik.

@fabianfett
Copy link
Member

Why would we want LambdaRuntime to be Sendable? it is not used directly by the users atm afaik.

@tomerd LambdaRuntime is supposed to be used by framework developers that wish to integrate with Lambda, since it allows adopters to hook it into their own lifecycle methods. The vapor lambda bridge works this way. For this reason LambdaRuntime is also part of the public API.

LambdaRuntime should be @unchecked Sendable, since it is thread safe by dispatching to the same eventLoop. This is currently not true for the start method, which needs to be fixed with the pattern:

    public func start() -> EventLoopFuture<Void> {
        if self.eventLoop.inEventLoop {
            return self.start0()
        } else {
            return self.flatSubmit { self.start0() }
        }
    }

    private func start0() -> EventLoopFuture<Void> {
        // current start implementation
    }

After that we should have marked everything in the public API as Sendable that needs to be Sendable.

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.

Awesome! I think we can make one comment a little clearer. Besides that, great work!

Sources/AWSLambdaRuntimeCore/LambdaRuntime.swift Outdated Show resolved Hide resolved
Co-authored-by: Fabian Fett <fabianfett@apple.com>
@tomerd tomerd merged commit 3c3529b into swift-server:main Apr 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔼 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.

None yet

2 participants