-
Notifications
You must be signed in to change notification settings - Fork 118
Add support for nonisolated(nonsending) by default #602
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
base: main
Are you sure you want to change the base?
Conversation
e5e6694 to
03dc004
Compare
on Streamable+Codable example
|
@mattmassicotte WDYT ? |
mattmassicotte
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not have enough context here to know if this is a reasonable path to take. However!
I remain surprised that these problems even came up. Moving from an isolated param to nonisolated(nonsending) is supposed to be source-compatible if a #isolation default was present and being used. And it seems like it was in all these cases...
| /// A writer object to write the Lambda response stream into. The HTTP response is started lazily. | ||
| /// before the first call to ``write(_:)`` or ``writeAndFinish(_:)``. | ||
| public protocol LambdaResponseStreamWriter { | ||
| public protocol LambdaResponseStreamWriter: Sendable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a Sendable requirement to a public protocol is definitely a breaking change. I do not understand the full implications of this, but it certainly has the potential to make it much harder for clients to conform to the protocol.
|
|
||
| /// Mock implementation of LambdaResponseStreamWriter for testing | ||
| final class MockLambdaResponseStreamWriter: LambdaResponseStreamWriter { | ||
| final actor MockLambdaResponseStreamWriter: LambdaResponseStreamWriter { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A common effect of adding a Sendable conformance to a protocol is pushing implementers to move from classes to actors. This is also a major change, because it now forces call sites to be async, as we see above. This, in turn, can introduce actor reentrancy problems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, but this is a mock just used for unit testing. I think we can afford the change in this context. Also, it doesn't break the public API.
The goal of this PR is to simplify the code and take advantage of new capabilities offered by the Swift 6.2 compiler, when possible.
However, one of the tenet of this project is to support the last three Swift version (6.0, 6.1, and 6.2 at the time of this writing). I therefore introduced this change for Swift 6.2 only, keeping existing code for Swift 6.0 and Swift 6.1.
When this change will be merged, we will create an issue to remember cleaning up this code when this project will not support 6.0 and 6.1 anymore (when 6.4 will be released, probably in Sept - Oct 2026)
Issue
Get rid of the
isolation: isolated (any Actor)? = #isolationparameter and rely on thenon isolated(nonsending)default instead.Description of changes
LambdaRuntimeClient+ChannelHandler.swift. One copy for Swift >= 6.2 and one copy for Swift < 6.2isolation: isolated (any Actor)? = #isolationon the >= 6.2 versionSendableconformance toLambdaResponseStreamWriter(read the API Breakage section hereunder)LambdaResponseStreamWriterTeststo transform mock classes to actorsNew/existing dependencies impact assessment
We think these changes might require a major version update, for multiple reasons.
First, by default, all public functions are now
nonisolated. This might break existing Lambda functions making assumptions about isolation status.Second, to allow the compiler to add
nonisolated(nonsending)by default, all parameters to closures must beSendable.This is unfortunately not the case for the low-level Lambda
handle()function (when used as a closure), becauseLambdaResponseStreamWriteris notSendable.Therefore, we added
Sendableconformance toLambdaResponseStreamWriterwhich might be a breaking change for anyone doing something fancy with it. Basic usage to this writer will not be affected.Existing Lambda functions using this function might break.
Two examples projects where affected by the feature flag:
HelloWorldNoTraitsandStreaming+Codable.HelloWorldNoTraits]: no change required to the project, addingSendabletoLambdaResponseStreamWriterwas enough to allow the project to compileStreaming+Codable]: we turned on the feature flag on the example project to solve compilation errors.We want to collect community feedback about your usage and trying to understand use cases where this change breaks your code.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.