Adopt Sendable throughout API to support -strict-concurrency=complete

[MahdiBM]

Motivation

When using -strict-concurrency=targeted, ClientMiddleware will emit warnings if an actor is conformed to it:

import OpenAPIRuntime
import Foundation

actor Middleware: ClientMiddleware {
    func intercept(
        _ request: Request,
        baseURL: URL,
        operationID: String,
        /// Need to add `@Sendable` or the warnings don't go away even with this PR. Code-completion won't add it.
        next: @Sendable (Request, URL) async throws -> Response
    ) async throws -> Response {
        try await next(request, baseURL)
    }
}

The intercept function will emit the following warning with the current implementation:

Sendability of function types in instance method 'intercept(_:baseURL:operationID:next:)' does not match requirement in protocol 'ClientMiddleware'

Repro package: https://github.com/MahdiBM/OAPIRClientWarningsRepro

You can change between this PR and the main branch by commenting out the dependency declarations i've already put in the Package.swift.
Then observe there are no warnings with this PR, unlike with the main branch.

Modifications

Generally adopt Sendable throughout the whole API to support -strict-concurrency=complete.
For the most part, this means using @sendable for closures, and using any Sendable instead of Any.

As an example for closures, currently we have:

public protocol ClientMiddleware: Sendable {
    func intercept(
        _ request: Request,
        baseURL: URL,
        operationID: String,
        next: (Request, URL) async throws -> Response
    ) async throws -> Response
}

With this PR we'll have:

public protocol ClientMiddleware: Sendable {
    func intercept(
        _ request: Request,
        baseURL: URL,
        operationID: String,
        next: @Sendable (Request, URL) async throws -> Response
        /// ~~~^ notice `@Sendable`
    ) async throws -> Response
}

Result

Safer Sendable code + being more ready for Swift 6 + no more warnings when using -strict-concurrency=targeted then conforming an actor to ClientMiddleware.

Test Plan

Adopted all tests to the changes and added the strict-concurrency flag to CI.

[czechboy0]

@MahdiBM Is this PR ready for us to review?

[czechboy0]

@swift-server-bot add to allowlist

[MahdiBM]

@czechboy0 yes 🙂

[czechboy0]

@MahdiBM Is this agains an option we could enable in Package.swift for the whole library, to prepare us for Swift 6, similar to apple/swift-openapi-generator#99? If we're making these changes, I'd like to make sure we don't regress the stricter concurrency mode in the future.

[MahdiBM]

So adding @sendable can only help for better concurrency checks.

Adding @preconcurrency though, we won't be able to remove it without it counting as a Breaking Change, which means it needs a major release, and that'll probably be when Swift 6 comes out, because Swift 6 comes with the default settings of basically -strict-concurrency=complete.

@preconcurrency is useful to ease people's migration to use concurrency stuff. It basically turns Sendable errors of declarations to just warnings.

I personally don't need it as my projects are for the most part already in compliance with at least -strict-concurrency=targeted. I reckon it might not be needed. I'm not sure at all. I'm curious to know what @FranzBusch thinks.

[FranzBusch]

Adding @preconcurrency though, we won't be able to remove it without it counting as a Breaking Change

That is not true. You can free add and remove that attribute whenever required. Especially if you version guard it. Overall, I agree though we should try to be strict concurrency warning free in this library.

@czechboy0 We are currently in the process of turning on strict concurrency checking in our CI in all our NIO libraries so feel free to add it here on the CI as well. You can simple pass this flag -strict-concurrency=complete to your swift invocation.

[czechboy0]

If we can go all the way to complete in one go, I'd prefer that as well. We'll have to see if there are any non-trivial things to fix up in the runtime library, and add the flag to the docker-compose file so that CI picks it up. @MahdiBM, can you try to see if complete works as well? Then we can get this PR into a shape where it passes all CI without warnings.

[MahdiBM]

@FranzBusch

You can free add and remove that attribute whenever required

is it not true that removing @preconcurrency from a function will make it so some possible warnings turn into errors? How is that acceptable without a major release?

[FranzBusch]

is it not true that removing @preconcurrency from a function will make it so some possible warnings turn into errors? How is that acceptable without a major release?

AFAIK, this will only add warnings and not errors. So it is safe to remove this. However, I don't think we should or need these attributes here in the first place

[MahdiBM]

Yeah, for this PR i'll be removing the @preconcurrencys. I'm just trying to see what i'm wrong about.

Imagine having this. It'll show a warning:

struct NotSendable {
    var a = ""
}

@preconcurrency
public func something(_: @Sendable () -> Void) { }

func another() async throws {
    var notSendable = NotSendable()
    something {
        notSendable.a = ""
///     ^~~~ Warning: Mutation of captured var 'notSendable' in concurrently-executing code; this is an error in Swift 6
    }
}

If you remove the @preconcurrency, the warning will turn into an error:

public func something(_: @Sendable () -> Void) { }

func another() async throws {
    var notSendable = NotSendable()
    something {
        notSendable.a = ""
///     ^~~~ Error: Mutation of captured var 'notSendable' in concurrently-executing code
    }
}

So if we publish the package with functions having @preconcurrency, then someone writes some code that has warning like above, removing @preconcurrency will make it so those warnings turn into errors ... I think that qualifies as a "breaking change"?

[MahdiBM]

@czechboy0

If we can go all the way to complete in one go, I'd prefer that as well. We'll have to see if there are any non-trivial things to fix up in the runtime library, and add the flag to the docker-compose file so that CI picks it up. @MahdiBM, can you try to see if complete works as well? Then we can get this PR into a shape where it passes all CI without warnings.

One thing to note is that we'll need to add the flag only in CI files like you requested. We can add it as .unsafeFlags(["-strict-concurrency=targeted"]) in Swift-settings of targets, but that'll throw weird errors for other libraries that use this package, if they rely on any tagged release. The good news is a fix for that is already merged and should be coming with Swift 5.9, and at that point we can just use that in the Package.swift: swiftlang/swift#66991

That being said, I'll check to see if we can use that flag without too many changes.

[FranzBusch]

So if we publish the package with functions having @preconcurrency, then someone writes some code that has warning like above, removing @preconcurrency will make it so those warnings turn into errors ... I think that qualifies as a "breaking change"?

The usage of @preconcurrency on a method is quite interesting. I don't think it is intended that this silences the error you are seeing without @preconcurrency. You should never be allowed the mutate a struct concurrently and I am quite surprised that the annotation is silencing the error. However, the reason why this is not breaking is that adopters of your package can always apply @preconcurrency on your modules and types when they import them and silence any warnings.

@czechboy0 maybe it is enough if we start using the new experimental feature flag in 5.9 and above only. So we don't have to pass flags in the CI scripts. In 5.9 and above we can just declare them in the experimentalFeatures section.

[MahdiBM]

@czechboy0 This should be ready.

• The RedactedHeadersStorage's purpose is to only silence strict-concurrency warnings that are due to mutation of static properties.

@FranzBusch

the reason why this is not breaking is that adopters of your package can always apply @preconcurrency on your modules

Honestly that might have been smart to have, but i just checked, and you can't silence the errors from functions of another module, using @preconcurrency on the the import. It just acts as if your @preconcurrency on the import is unrelated and shows a warning to remove @preconcurrency.

I don't think it is intended that this silences the error you are seeing without @preconcurrency

I think it's intended as we figured in the ongoing work of @0xTim to make Vapor stuff more Sendable.
It helps converting the old functions that have not-@sendable closures, to functions with @sendable closures. It enabled Tim to make those closures @sendable, while marking the functions as @preconcurrency so the newly-@Sendable-marked closures don't result in code-breakage of users (as we learnt the hard way when users complained about their code breakages)

[FranzBusch]

I think it's intended as we figured in the ongoing work of @0xTim to make Vapor stuff more Sendable.
It helps converting the old functions that have not-https://github.com/sendable closures, to functions with https://github.com/sendable closures. It enabled Tim to make those closures https://github.com/sendable, while marking the functions as @preconcurrency so the newly-@Sendable-marked closures don't result in code-breakage of users (as we learnt the hard way when users complained about their code breakages)

Sure for existing code this might be the case and you have to handle this. However, this package has not yet hit a 1.0.0 and we can safely introduce this without adding @preconcurrency

[0xTim]

Chiming in to add as a new package there should be no @preconcurrency in here 🙂

[MahdiBM]

However, this package has not yet hit a 1.0.0 and we can safely introduce this without adding @preconcurrency

Yeah you're right in that sense, i've already removed all the @preconcurrencys 🙂

[czechboy0]

That said - we do guarantee stability between minor versions until we hit 1.0, so this PR would be considered breaking the API. That's okay, we just want to mark it as such and make sure it's rolled out in a new minor version, instead of a patch.

[czechboy0]

@MahdiBM, please also add the CLI flag for the strict concurrency checking to the CI script (the docker-compose file), to see that it's all passing before we start reviewing the PR again, thanks! 🙏

[czechboy0]

@MahdiBM please lmk once the CI is green again and I'll do one more review 🙏

[MahdiBM]

@czechboy0 the PR (expectedly) contains some breaking changes so i can't really make the CI green 😅:

15 breaking changes detected in OpenAPIRuntime:
  💔 API breakage: constructor OpenAPIValueContainer.init(unvalidatedValue:) has parameter 0 type change from Any? to (Swift.Sendable)?
  💔 API breakage: var ClientError.operationInput has declared type change from Any to Swift.Sendable
  💔 API breakage: accessor ClientError.operationInput.Get() has return type change from Any to Swift.Sendable
  💔 API breakage: accessor ClientError.operationInput.Set() has parameter 0 type change from Any to Swift.Sendable
  💔 API breakage: constructor ClientError.init(operationID:operationInput:request:baseURL:response:underlyingError:) has parameter 1 type change from Any to Swift.Sendable
  💔 API breakage: var ServerError.operationInput has declared type change from Any? to (Swift.Sendable)?
  💔 API breakage: accessor ServerError.operationInput.Get() has return type change from Any? to (Swift.Sendable)?
  💔 API breakage: accessor ServerError.operationInput.Set() has parameter 0 type change from Any? to (Swift.Sendable)?
  💔 API breakage: var ServerError.operationOutput has declared type change from Any? to (Swift.Sendable)?
  💔 API breakage: accessor ServerError.operationOutput.Get() has return type change from Any? to (Swift.Sendable)?
  💔 API breakage: accessor ServerError.operationOutput.Set() has parameter 0 type change from Any? to (Swift.Sendable)?
  💔 API breakage: constructor ServerError.init(operationID:request:requestMetadata:operationInput:operationOutput:underlyingError:) has parameter 3 type change from Any? to (Swift.Sendable)?
  💔 API breakage: constructor ServerError.init(operationID:request:requestMetadata:operationInput:operationOutput:underlyingError:) has parameter 4 type change from Any? to (Swift.Sendable)?
  💔 API breakage: func UniversalClient.send(input:forOperation:serializer:deserializer:) has generic signature change from <OperationInput, OperationOutput> to <OperationInput, OperationOutput where OperationInput : Swift.Sendable, OperationOutput : Swift.Sendable>
  💔 API breakage: func UniversalServer.handle(request:with:forOperation:using:deserializer:serializer:) has generic signature change from <APIHandler, OperationInput, OperationOutput where APIHandler : Swift.Sendable> to <APIHandler, OperationInput, OperationOutput where APIHandler : Swift.Sendable, OperationInput : Swift.Sendable, OperationOutput : Swift.Sendable>

They are all complaining about Sendable.

[czechboy0]

Ah ofc, I didn't realize. Let me talk with @simonjbeaumont tomorrow about how we'll land and release this breaking change.

[simonjbeaumont]

Ah ofc, I didn't realize. Let me talk with @simonjbeaumont tomorrow about how we'll land and release this breaking change.

I already labelled this PR as semver/minor so I think we just release and cut a new version. We can highlight in the release notes that this is breaking and we already advise in our docs that folks should be using upToNextMinor during the pre-1.0 period if they want to avoid breaking changes.

The current state of CI is misleading. The integration test for this PR is actually passing. It's being run in soundness right now because this PR hasn't been rebased since #23 was merged. This means the Github status for this PR is currently showing a pipeline for the integration test that is failing because the Compose "service" the pipeline is trying to run doesn't exist on this branch.

I'll update this branch, which should then at least make the Github checks pass for this PR.

However, the integration test does produce warnings (because there's a conversion of a non-sendable closure to a sendable one), which I imagine would be an error for an adopter if they were running with a stricter compiler flag.

[MahdiBM]

Let me investigate this. But for now, the good news is there is no stricter concurrency flag and i doubt there will be one.

If you want to cut a release and do release notes, i can also go the existential-any PR first, so you can do them together if it matters (probably/hopefully that won't need as much time/conversation as this, as it's just some anys that the compiler will expect you to explicitly use)

[MahdiBM]

So considering the fact that i don't think the integration-tests test-package is public, and also my lack of knowledge about the generator package, i couldn't come to a definite conclusion. But:

• It appears the openapi-generator package is generating some non-Sendable stuff? (APIHandler.getGreeting)
• If that's true, then it's a concern and should be resolved.
• But even so, the openapi-generator is set to use up to the "next-minor" version of this package, so merging this PR can't break that.
• The integration tests are running without any strict-concurrency flag. If someone is using the generator with stricter concurrency, their code will likely break honestly IF we don't fix that problem in the openapi-generator before allowing the generator to pull higher minor versions.
  • I reckon this might have been what @simonjbeaumont meant when saying "I imagine would be an error for an adopter if they were running with a stricter compiler flag"
• Weird thing is the same thing is not happening with the PetstoreConsumerTests 🤔

[simonjbeaumont]

Thanks @MahdiBM. I think you've managed to glean what's going on. Let me summarise for you.

We have an IntegrationTest Swift package which is colocated in the swift-openapi-generator repository. It is a simple package that has dependencies on the generator plugin and the runtime library.

In CI we clone this package and use swift package edit to override the dependency on the package that the PR is for. Using swift package edit will override any upToNextMinor constraint IIUC.

So, as expected, for this PR we see the following in the CI logs:

[620/621] Compiling Server Server.swift
/code/tmp.run-integration-test.sh.FMusHSr3Pf.noindex/swift-openapi-generator/IntegrationTest/.build/plugins/outputs/integrationtest/Server/OpenAPIGenerator/GeneratedSources/Server.swift:45:31: warning: converting non-sendable function value to '@Sendable (APIHandler) -> ((Operations.getGreeting.Input) async throws -> Operations.getGreeting.Output)' may introduce data races
            using: APIHandler.getGreeting,

Depending on the PR this information might be gating or just informational, just like the api breakage pipeline.

In this case it just serves to confirm that we need to bump the minor version (it'd be major if we were 1.0, ofc) and folks can expect a potentially breaking change if they choose to upgrade. And hopefully the upgrade will be an explicit action on the adopters part because they'll hopefully be using upToNextMinor.

Specifically, on your notes:

• It appears the openapi-generator package is generating some non-Sendable stuff? (APIHandler.getGreeting)
• If that's true, then it's a concern and should be resolved.
• But even so, the openapi-generator is set to use up to the "next-minor" version of this package, so merging this PR can't break that.

Correct. I guess the release flow here would need to be:

1. Merge this PR and cut a 0.2.0 release of runtime.
2. Update generator to use 0.2.0 runtime and determine whether this also needs a minor bump.
3. Also update the integration test.

• The integration tests are running without any strict-concurrency flag. If someone is using the generator with stricter concurrency, their code will likely break honestly IF we don't fix that problem in the openapi-generator before allowing the generator to pull higher minor versions.

  • I reckon this might have been what @simonjbeaumont meant when saying "I imagine would be an error for an adopter if they were running with a stricter compiler flag"

Right, that's what I meant. I think we should probably add that to the integration test when we can. Is this something you wanted to chase down as you shepherd this feature through?

[MahdiBM]

I think we should probably add that to the integration test when we can. Is this something you wanted to chase down as you shepherd this feature through?

I think that means updating the Dockerfile with the strict concurrency flag?

[simonjbeaumont]

I think we should probably add that to the integration test when we can. Is this something you wanted to chase down as you shepherd this feature through?

I think that means updating the Dockerfile with the strict concurrency flag?

Right. My expectation is, if we were to do that in this PR, we'd see it fail. Might be worth doing to hammer home the nature of this change (at least we'd see that pipeline and the API breakage pipeline agree).

[czechboy0]

lgtm

[MahdiBM]

hmm adding the flag did not make the tests fail which is weird 🤔

But the good news is now that i'm testing the openapi-generator with this branch of openapi-runtime, i can at least see the warnings.

[MahdiBM]

I wonder if this a diagnosis issue by Swift:

using: APIHandler.updatePet,
/// Warning: Converting non-sendable function value to '@Sendable (APIHandler) -> ((Operations.updatePet.Input) async throws -> Operations.updatePet.Output)' may introduce data races

but:

using: { APIHandler.updatePet($0) },
/// No warnings!

Both with -strict-concurrency=complete.

[MahdiBM]

After more investigations, i think it's either a Swift diagnosis error that there are those warnings, or it's a correct warning that we can't do much about anyway, other than working around it like in the comment above.

The function is declared in APIProtocol:

public protocol APIProtocol: Sendable {
...

func updatePet(_ input: Operations.updatePet.Input) async throws -> Operations.updatePet.Output
}

Then UniversalServer uses the function like so:

using: APIHandler.updatePet,

And there is nothing you can do to make the warning go away, without touching what is in UniversalServer. Even marking everything (functions, types, etc...) with @Sendable doesn't help (most of the stuff are already Sendable anyway).
I think we can just use the workaround above to suppress the warnings after this PR is merged.
EDIT:
For more info, the using parameter is defined as:

using handlerMethod: @Sendable @escaping (APIHandler) -> ((OperationInput) async throws -> OperationOutput),

[FranzBusch]

I think this is a known bug in Sendable checking right now. Just to be sure, do you mind creating a small reproducer and filing an issue over in the Swift repository

[MahdiBM]

I found an existing issue for very similar behavior, it should be enough considering we now know there is an issue for it: swiftlang/swift#64388

[MahdiBM]

Hmm i guess i'll file a new issue considering what we're seeing here and what is in that issue have some differences, although the warning is the same.

[MahdiBM]

Filed the issue.

To reiterate and be clear, the warnings seem to always be/remain completely harmless, although we should still resolve them in openapi-generator after this PR is merged.

[simonjbeaumont]

Thanks @MahdiBM for pushing this over the line!

Discussed this with the wider team and changes to sendable aren't currently considered breaking from a semver perspective. @FranzBusch can you confirm?