[swift] refactor global fields to local fields in API classes

[kalinjul]

Generated Swift code used global static variables for important state like customHeaders and requestBuilderFactory (see APIs.mustache).
This likely introduces race conditions if one needs to set a custom requestBuilderFactory or header for a single request, as it is not guaranteed that no other Thread/Task re-sets this global var concurrently.

This PR adds local fields in generated Api classes that are initialized with the existing static fields by default.
Api calls are no longer class funcs.

You can now create two instances of an API using different RequestBuilders, headers, and so on. In other platforms such as kotlin or java, this was always possible using different ApiClients.

For backwards compatibility, you may just use the constructor with it's default values, which pulls in the static values just as before.

fixes #17377

TODO: This currently only adresses swift5, swift6 in progress.

PR checklist

• Read the contribution guidelines.
• Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
• Run the following to build the project and update samples:

  ./mvnw clean package || exit
  ./bin/generate-samples.sh ./bin/configs/*.yaml || exit
  ./bin/utils/export_docs_generators.sh || exit
  

  (For Windows users, please run the script in Git BASH)
  Commit all changed files.
  This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
  These must match the expectations made by your contribution.
  You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
  IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
• File the PR against the correct branch: master (upcoming 7.x.0 minor release - breaking changes with fallbacks), 8.0.x (breaking changes without fallbacks)
• If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

[4brunu]

Hi @kalinjul.
I would like to suggest to you checking the swift 6 generator and consider contributing to it.
Swift 6 already supports different headers, base url, etc, per request, although it still uses the global variables, because the variables were moved into a singleton.
But the swift 6 generator is aligned with what you are trying to archive here.
Please check if it fills yours needs or if still needs some tweaking.

[rhysm94]

Sorry to perform thread/issue necromancy here but I believe, from the Swift 6 generated code I’ve seen, that this is still a problem.
The issue is that two separate async contexts/threads in the app could still simultaneously try and write to these properties, even via the singleton, and cause a crash. I don’t think making this a singleton fixes the issue. All the @unchecked Sendable does is hide the issue by telling the compiler “no, don’t worry, this is totally safe” when it’s not.

These properties need to be protected by some synchronisation mechanism – either making the APIConfiguration type an actor, or using something like a lock (as in SynchronizedDictionary) to protect them. iOS 18 and above ships with a Mutex type, and below that you can approximate it by looking at something like the LockIsolated type in Point-Free’s Concurrency-Extras library. Obviously, given the backwards compatibility needed by the generated code, I think it probably makes more sense to either wrap these properties in an actor or wrap the individual properties/bundle them into a struct that’s protected by LockIsolated, ensuring that these properties are correctly isolated, and attempting to mutate them from different threads at the same time won’t cause memory corruption and crash the application.

[4brunu]

@rhysm94 would you be willing to submit a PR? I can help with the review

[4brunu]

Hi @rhysm94 I have been thinking about your comment, at the time I didn't had much time to respond, but I think I can add more detail.
First of all, you are right, I agree with you.
Ideally we should not have any @unchecked Sendable, but when we have them, we should make sure it's safe by using locks.
I'm a bit afraid that use an actor will result in a lot of changes, so maybe using a lock it's the easier way to improve this without many changes?
Over the years I made an effort to avoid adding external dependencies, so I would like to avoid using the LockIsolated.
What do you think of this?

[rhysm94]

Hi @rhysm94 I have been thinking about your comment, at the time I didn't had much time to respond, but I think I can add more detail.

First of all, you are right, I agree with you.

Ideally we should not have any @unchecked Sendable, but when we have them, we should make sure it's safe by using locks.

I'm a bit afraid that use an actor will result in a lot of changes, so maybe using a lock it's the easier way to improve this without many changes?

Over the years I made an effort to avoid adding external dependencies, so I would like to avoid using the LockIsolated.

What do you think of this?

Hi @4brunu! Sorry, also not had much time to respond.
Yeah, I think it's fair to avoid external dependencies here, where possible. I think we can just recreate the LockIsolated idea locally in the generated code, and use that.

The extra important thing would be to expose the fields or the entire state as a LockIsolated to avoid data races, not just making this "safe" (if you look at the previously linked documentation, you'll see it offers a withValue method to ensure everything happens transactionally).

[rhysm94]

I should say - I'm happy to look at raising a PR into this soon.

[4brunu]

I raised the minimum iOS version to iOS 13 so that we can use the swift concurrent features without availability checks.
#22802

I think we could raise it even more, to something like iOS 15, but I'm not sure that we would gain something with that.

Yeah, I think it's fair to avoid external dependencies here, where possible. I think we can just recreate the LockIsolated idea locally in the generated code, and use that.

Yes, I agree we should create something locally in the generated code.
I'm not sure if we can copy LockIsolated because of possible license issues (I'm not an expert on this), but it shouldn't be complicated to create a wrapper class with locks.

The extra important thing would be to expose the fields or the entire state as a LockIsolated to avoid data races, not just making this "safe" (if you look at the previously linked documentation, you'll see it offers a withValue method to ensure everything happens transactionally).

Could you please expand on this?

I should say - I'm happy to look at raising a PR into this soon.

Thanks for your help.
Ideally we should keep the public API as similar as possible, to make the transition easy, or ideally seamlessly.

[rhysm94]

Yeah, I think it's fair to avoid external dependencies here, where possible. I think we can just recreate the LockIsolated idea locally in the generated code, and use that.

Yes, I agree we should create something locally in the generated code.

I'm not sure if we can copy LockIsolated because of possible license issues (I'm not an expert on this), but it shouldn't be complicated to create a wrapper class with locks.

Yeah, understandable. I'll double check the licensing for LockIsolated in particular, but realistically we can also "copy" Apple's Mutex type, which has a near identical API to LockIsolated too.

The extra important thing would be to expose the fields or the entire state as a LockIsolated to avoid data races, not just making this "safe" (if you look at the previously linked documentation, you'll see it offers a withValue method to ensure everything happens transactionally).

Could you please expand on this?

Yeah, so I'll link to the LockIsolated documentation that explains this particularly well, but the gist is this:

If I have field counter that is exposed as an Int, but is backed by a Mutex-style type, and publicly exposed using the counterMutex.value property, you open up for risk of data races.

I could write code that does the following:

let previousCount = counter
counter = previousCount + 1

then we have the risk of a data race, between these two lines of code. After I capture the previous value of counter, anything else could have incremented or changed counter, meaning I could now be setting counter to some old value again.

The way around this is to make sure access and mutation happens transactionally, using a withValue method that gives you the current value in the closure, so you read and write in one atomic transaction. e.g.

counter.withValue { $0 += 1 }

This withValue method is wrapped entirely in the lock, meaning nothing else can change the value from under you.

The documentation for ConcurrencyExtras has a really good explanation for this problem, any why using the setValue method should largely not be used, in favour of the withValue closure: https://github.com/pointfreeco/swift-concurrency-extras/blob/5a3825302b1a0d744183200915a47b508c828e6f/Sources/ConcurrencyExtras/LockIsolated.swift#L51

I should say - I'm happy to look at raising a PR into this soon.

Thanks for your help.

Ideally we should keep the public API as similar as possible, to make the transition easy, or ideally seamlessly.

I'll attempt this, maybe if we can use deprecated properties or something? Not sure. We'll try and work something out, even if it's a clear migration path to the new pattern!

[4brunu]

Yeah, understandable. I'll double check the licensing for LockIsolated in particular, but realistically we can also "copy" Apple's Mutex type, which has a near identical API to LockIsolated too.

Copy the Apple's Mutex is a good ideia, because it will be easy to migrate in the future, when we can use Apple's Mutex.

Yeah, so I'll link to the LockIsolated documentation that explains this particularly well, but the gist is this:

If I have field counter that is exposed as an Int, but is backed by a Mutex-style type, and publicly exposed using the counterMutex.value property, you open up for risk of data races.

I could write code that does the following:

let previousCount = counter
counter = previousCount + 1

then we have the risk of a data race, between these two lines of code. After I capture the previous value of counter, anything else could have incremented or changed counter, meaning I could now be setting counter to some old value again.

The way around this is to make sure access and mutation happens transactionally, using a withValue method that gives you the current value in the closure, so you read and write in one atomic transaction. e.g.

counter.withValue { $0 += 1 }

This withValue method is wrapped entirely in the lock, meaning nothing else can change the value from under you.

The documentation for ConcurrencyExtras has a really good explanation for this problem, any why using the setValue method should largely not be used, in favour of the withValue closure: https://github.com/pointfreeco/swift-concurrency-extras/blob/5a3825302b1a0d744183200915a47b508c828e6f/Sources/ConcurrencyExtras/LockIsolated.swift#L51

I see, it's similar to NSLock. Looks good to me.

I'll attempt this, maybe if we can use deprecated properties or something? Not sure. We'll try and work something out, even if it's a clear migration path to the new pattern!

Thanks