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

Better concurrency support in iOS API Manager #45

Merged

Conversation

gazreese
Copy link
Contributor

@gazreese gazreese commented Dec 5, 2023

Description

As highlighted in another PR the current iOS client didn't have good concurrency support. This was verified with a failing unit test, then subsequently fixed using GCD queues rather than the NSLock model suggested in the other PR.

The use of the queues rather than NSLock was to increase maintainability going forward as more iOS developers would be familiar with queues than Posix thread locks.

The original API Manager was also using the whole URLSessionTask object as the key to its dictionary, and this task contains the whole request and response. It's a big object to be copying around and didn't need to be used as it already has a unique identifier for this purpose. I'm not sure if this was leaking but it's probably not the best way to go about it and we don't need access to the task as it's managed by URLSession.

Also converted to the Swift Data class from NSMutableData as per the suggestions in the original PR.

Type of Change

  • ✨ New feature (non-breaking change which adds functionality)
  • 🛠️ Bug fix (non-breaking change which fixes an issue)
  • ❌ Breaking change (fix or feature that would cause existing functionality to change)
  • 🧹 Code refactor
  • ✅ Build configuration change
  • 📝 Documentation
  • 🗑️ Chore

Example/FlagsmithClient/AppDelegate.swift Outdated Show resolved Hide resolved
FlagsmithClient/Tests/APIManagerTests.swift Outdated Show resolved Hide resolved
FlagsmithClient/Classes/Internal/APIManager.swift Outdated Show resolved Hide resolved
@matthewelwell matthewelwell merged commit 695bcdd into Flagsmith:main Mar 11, 2024
2 checks passed
@matthewelwell matthewelwell mentioned this pull request Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants