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

Support NSUbiquitousKeyValueStore #136

Merged
merged 13 commits into from
Apr 20, 2024
Merged

Conversation

hank121314
Copy link
Collaborator

Summary

This PR fixes: #75

Create a Defaults.iCloud class which facilitates the synchronization of keys between the local UserDefaults and NSUbiquitousKeyValueStore.
This class utilizes a TaskQueue to execute synchronization tasks sequentially, ensuring proper order and coordination.
Internally, Defaults.iCloud contains an AtomicSet that indicates the synchronization status of each key. This set prevents observation triggers when synchronization is being executed.

And I also create a new protocol Defaults.KeyValueStore which contains essential properties for synchronizing a key value store. It can be useful when mocking a test or even support another key value data source.

Thanks

Please feel free to critique, and thanks for your code review 😄 .
Any feedback on the PR is welcomed and appreciated 🙇 !

@sindresorhus
Copy link
Owner

Wow. This is really awesome! 🎉

Sources/Defaults/Defaults+iCloud.swift Outdated Show resolved Hide resolved
Sources/Defaults/Defaults+iCloud.swift Outdated Show resolved Hide resolved
Sources/Defaults/Defaults+iCloud.swift Outdated Show resolved Hide resolved
Sources/Defaults/Defaults+iCloud.swift Outdated Show resolved Hide resolved
Comment on lines 356 to 361
private func addObserver(_ key: Defaults.Keys) {
backgroundQueue.sync {
key.suite.addObserver(self, forKeyPath: key.name, options: [.new], context: nil)
}
}

private func removeObserver(_ key: Defaults.Keys) {
backgroundQueue.sync {
key.suite.removeObserver(self, forKeyPath: key.name, context: nil)
}
}

@_documentation(visibility: private)
// swiftlint:disable:next block_based_kvo
override public func observeValue(
forKeyPath keyPath: String?,
of object: Any?,
change: [NSKeyValueChangeKey: Any]?, // swiftlint:disable:this discouraged_optional_collection
context: UnsafeMutableRawPointer?
) {
guard
let keyPath,
let object,
object is UserDefaults,
let key = keys.first(where: { $0.name == keyPath }),
!atomicSet.contains(key)
else {
return
}

backgroundQueue.async {
self.recordTimestamp(.local)
await self.syncKey(key, .local)
}
}
Copy link
Owner

@sindresorhus sindresorhus May 22, 2023

Choose a reason for hiding this comment

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

It would be nicer to wrap this up into a separate utility that is decoupled from Defaults.iCloud. And also have it return an async stream. I generally prefer to wrap legacy APIs in a nicer interface. This also helps keep code clean.


- Note: `source` should be specify if `key` has not been added to `Defaults.iCloud`.
*/
public static func syncKeys(_ keys: Defaults.Keys..., source: DataSource? = nil) {
Copy link
Owner

Choose a reason for hiding this comment

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

What's the use-case for this method? Do we really need to expose it?

/**
Synchronize all of the keys that have been added to Defaults.iCloud.
*/
public static func syncKeys() {
Copy link
Owner

Choose a reason for hiding this comment

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

It's a bit confusing to have both .sync() and .syncKeys() and it's not immediately clear when you would want to use either. Can we just make .sync() do both? Or am I missing something?

Copy link
Collaborator Author

@hank121314 hank121314 May 29, 2023

Choose a reason for hiding this comment

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

syncKeys() will create synchronization task for all keys and push it into backgroundQueue.
await sync() will wait until all synchronization tasks done, maybe we can call it flush()?

Copy link
Owner

Choose a reason for hiding this comment

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

flush() or syncWithoutWaiting() would work.

But my question still stands, do we really need to expose this to the user?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I saw some requests in ArtSabintsev/Zephyr#32.
With await flush(), users will know that all pending synchronizations are done.

Copy link
Owner

Choose a reason for hiding this comment

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

I think it should be syncWithoutWaiting() and await sync(). That's the most explicit names I can think of.

return .remote
}

return localTimestamp.timeIntervalSince1970 > remoteTimestamp.timeIntervalSince1970 ? .local : .remote
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
return localTimestamp.timeIntervalSince1970 > remoteTimestamp.timeIntervalSince1970 ? .local : .remote
return localTimestamp > remoteTimestamp ? .local : .remote

Sources/Defaults/Utilities.swift Show resolved Hide resolved
Comment on lines 59 to 68
public protocol _DefaultsKeyValueStore {
func object(forKey aKey: String) -> Any?
func set(_ anObject: Any?, forKey aKey: String)
func removeObject(forKey aKey: String)
@discardableResult
func synchronize() -> Bool
}
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
public protocol _DefaultsKeyValueStore {
func object(forKey aKey: String) -> Any?
func set(_ anObject: Any?, forKey aKey: String)
func removeObject(forKey aKey: String)
@discardableResult
func synchronize() -> Bool
}
public protocol _DefaultsKeyValueStore {
func object(forKey key: String) -> Any?
func set(_ object: Any?, forKey key: String)
func removeObject(forKey key: String)
@discardableResult
func synchronize() -> Bool
}

Comment on lines 337 to 344
let semaphore = DispatchSemaphore(value: 0)

queueContinuation?.yield {
await task()
semaphore.signal()
}

semaphore.wait()
Copy link
Owner

Choose a reason for hiding this comment

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

This will deadlock if the task accidentally calls something that also calls .sync() (nested calls).

According to ChatGPT, you could do something like this:

final class TaskQueue {
	typealias AsyncTask = @Sendable () async -> Void
	private var queueContinuation: AsyncStream<AsyncTask>.Continuation?
	private let queue: DispatchQueue

	init(priority: TaskPriority? = nil) {
		queue = DispatchQueue(label: "com.example.taskqueue", attributes: .concurrent)
		let taskStream = AsyncStream<AsyncTask> { queueContinuation = $0 }

		Task.detached(priority: priority) {
			for await task in taskStream {
				await task()
			}
		}
	}

	deinit {
		queueContinuation?.finish()
	}

	func async(_ task: @escaping AsyncTask) {
		queue.async {
			self.queueContinuation?.yield(task)
		}
	}

	func sync(_ task: @escaping AsyncTask) {
		let semaphore = DispatchSemaphore(value: 0)

		queue.async {
			if DispatchQueue.getSpecific(key: self.queue.key) == self.queue.id {
				Task {
					await task()
					semaphore.signal()
				}
			} else {
				self.queueContinuation?.yield {
					await task()
					semaphore.signal()
				}
			}
		}

		semaphore.wait()
	}

	func flush() async {
		await withCheckedContinuation { continuation in
			queue.async {
				self.queueContinuation?.yield {
					continuation.resume()
				}
			}
		}
	}
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's true, I think we should deprecate sync.
Would it be acceptable to use TaskQueue.async for updating remote key changes?

Copy link
Owner

Choose a reason for hiding this comment

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

The async queue is concurrent, so it maybe cause ordering problems.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

TaskQueue.async is not concurrent; it will run similarly to DispatchQueue.async.
I believe it should not have ordering problems.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Btw, if calling await flush in TaskQueue.async, it will also cause deadlock.
Maybe we should emit a Xcode runtime warning to prevent user doing this.

ex.

let queue = TaskQueue(priority: .background)
queue.async {
    print("1")
    queue.async {
        print("2")
    }
    await queue.flush()
}
await queue.flush()
// => 1 "2" will never print.

Copy link
Owner

Choose a reason for hiding this comment

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

Btw, if calling await flush in TaskQueue.async, it will also cause deadlock.
Maybe we should emit a Xcode runtime warning to prevent user doing this.

Users don't have access to TaskQueue though. It's an internal type.

@hank121314
Copy link
Collaborator Author

hank121314 commented May 26, 2023

It is challenging to change the Key-Value Observing (KVO) mechanism of UserDefaults to asyncstream.

Here is a demonstration of how Defaults.iCloud works.

flowchart TD
    A("UserDefaults \n Changes \n ex.Defaults[key] = 0")
    B("NSUbiquitousKeyValueStore \n didChangeExternallyNotification")
    C("Defaults.iCloud.syncKeys()")
    D[\"Push \n Synchronization \n Task \n Sequentially"/]
    E(["Executing Tasks \n Task.detached"])
    F(["All Tasks Complete"])
    A & B & C --> D --> E -- "await Defaults.iCloud.sync()" --> F
Loading

If we change the observation to asyncstream, there is a need to create a task to consume the asyncstream. Since the user's change is not in the same task as the asyncstream, it is difficult to ensure that the synchronization is completed synchronously.

ex.

Task.detached {
	for await change in updates {
		print("\(change)")
	}
}

Defaults[key] = 0
Defaults[key] = 1
await Defaults.iCloud.sync()

// => 0
// await Defaults.iCloud.sync() get call
// => 1

@sindresorhus
Copy link
Owner

Understood. It's fine. We can use KVO then. Just add a todo to look into using async stream when Swift supports custom executors.

@hank121314
Copy link
Collaborator Author

I believe it might be worth considering waiting for the release of Swift 5.9, which will include the implementation of Custom Actor Executors.
Once it's available, we can transition all Key-Value Observing (KVO) and notifications to utilize AsyncStream.

@@ -327,21 +338,9 @@ final class TaskQueue {
Queue a new asynchronous task.
*/
func async(_ task: @escaping AsyncTask) {
lock.lock()
Copy link
Owner

Choose a reason for hiding this comment

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

It's generally better to have a .with type method to make sure the locked resource is always unlocked. Like: https://developer.apple.com/documentation/os/osallocatedunfairlock/4061628-withlock

@sindresorhus
Copy link
Owner

Is the custom actor executors feature backdeployed? If not, we would have to target macOS 14, which does not make sense.

import Combine
import Foundation

/// Represent different data sources available for synchronization.
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
/// Represent different data sources available for synchronization.
/**
Represent different data sources available for synchronization.
*/

Comment on lines 20 to 14
private enum SyncStatus {
case start
case isSyncing
case finish
}
Copy link
Owner

Choose a reason for hiding this comment

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

This would be more idiomatic Swift:

Suggested change
private enum SyncStatus {
case start
case isSyncing
case finish
}
private enum SyncStatus {
case idle
case syncing
case completed
}

Not sure idle is correct though.

/**
The singleton for Defaults's iCloudSynchronizer.
*/
static var shared = Defaults.iCloudSynchronizer(remoteStorage: NSUbiquitousKeyValueStore.default)
Copy link
Owner

Choose a reason for hiding this comment

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

The name is not correct. It makes it sound like it's the singleton of iCloud.

Suggested change
static var shared = Defaults.iCloudSynchronizer(remoteStorage: NSUbiquitousKeyValueStore.default)
static var synchronizer = Defaults.iCloudSynchronizer(remoteStorage: NSUbiquitousKeyValueStore.default)

@hank121314
Copy link
Collaborator Author

Is the custom actor executors feature backdeployed? If not, we would have to target macOS 14, which does not make sense.

SerialExecutor is back deployed to macOS 10.15+, but some of its methods is deprecated and new methods only target to macOS 14+. And I am also misreading the proposal, we must wait until the Specifying Task executors is completed.

@hank121314
Copy link
Collaborator Author

Btw, my old MacBook can not install macOS 14+ and Xcode 15+, so I can not develop some features which related to swift 5.9 😭 . Will buy the M3 MacBook until it release in Taiwan.

@Kinark
Copy link

Kinark commented Dec 12, 2023

Any updates on this? It's a really awesome PR :/

@hank121314
Copy link
Collaborator Author

hank121314 commented Dec 29, 2023

I want to check a behavior when initialized with iCloud: true.
Currently, we push synchronization from the local data source triggered by suite.register.
However, I am concerned that this may overwrite remote data when installing on a new device.
Perhaps it would be better to add UserDefaults observation after suite.register and let the user manually call Defaults.syncWithoutWaiting (which compares timestamps between the two data sources).
What do you think?

@sindresorhus
Copy link
Owner

sindresorhus commented Dec 30, 2023

However, I am concerned that this may overwrite remote data when installing on a new device.
Perhaps it would be better to add UserDefaults observation after suite.register and let the user manually call Defaults.syncWithoutWaiting (which compares timestamps between the two data sources).
What do you think?

I would prefer if users don't have to call anything manually. Can we wait until the sync (fetching remote data) is done before pushing the local data to remote?

Maybe also look at how Zephyr handles this.

@hank121314
Copy link
Collaborator Author

Seems Zephyr also facing the same issue. ArtSabintsev/Zephyr#50 (comment)

I have a thought to resolve this issue, but it might involve some trade-offs.
The current implementation only records the latest timestamp when data is set in the data source, using only one key.
If we can record the latest timestamp for all keys, we can ensure whether a specific key needs to be synced or not.
However, the trade-off is a 2x space allocation with both remote and local storage.

@sindresorhus
Copy link
Owner

Local storage is not a problem, but synced storage is a problem as it's limited to 1 Kb in total. Is there a way to use timestamps for each key but not sync them?

@hank121314
Copy link
Collaborator Author

Is there a way to use timestamps for each key but not sync them?

Oh, this is a good idea.
For this use case, where a user has a fresh new device with some data in iCloud Storage, we can consider the iCloud Storage as the latest data since there is no corresponding timestamp key in their local storage.

Will try to implement this in a few days! Thank you 🙌 !

@hank121314
Copy link
Collaborator Author

hank121314 commented Jan 13, 2024

I tried the method mentioned above but noticed that it may not cover all use cases.

If a user has multiple Defaults.Keys with iCloud set to true, after syncing the first key, the remote timestamp is recorded. When syncing the second key, we cannot be sure about the state of the data source.

For example.

// Choose `local` data source, sync `name` to `remoteStorage`, record `remoteStorage` timestamp.
let name = Defaults.Key<String>("name", default: "0", iCloud: true)
// `remoteStorage` have timestamp record and local timestamp is empty so we will choose `remote` data source, sync `quality` to `localStorage`, but the data source should be `local` instead.
let quality = Defaults.Key<Double>("quality", default: 0.0, iCloud: true)

Considering the scenario, it seems necessary to maintain timestamp records in both remoteStorage and localStorage to accurately determine the appropriate data source 😭 .

And considering the limitation of 1024 keys in remoteStorage, a potential solution could be to consolidate the value and timestamp into a single key?

For example.

class iCloudKey {
	let value: Any?
	let timestamp: Date?
}

@sindresorhus
Copy link
Owner

Considering the scenario, it seems necessary to maintain timestamp records in both remoteStorage and localStorage to accurately determine the appropriate data source 😭 .

You are right. I don't see any way around this either.

@hank121314
Copy link
Collaborator Author

So it is necessary to maintain the timestamp in both storages.

For localStorage, where there is no limitation, we can introduce another key with a prefix (__DEFAULTS__synchronizeTimestamp) to record the timestamp.
As for remoteStorage, considering the 1024 keys limitation, we can combine the value and timestamp into a single key and stored it in remoteStorage.

Is this an acceptable solution?

@sindresorhus
Copy link
Owner

sindresorhus commented Jan 26, 2024

Correct

For the remote storage we should try to make it take the least amount of space, because there is also a total limit of 1MB storage. Maybe we can convert the timestamp to fixed-length data, serialize the value to data too, combine them, and store them both in one Data value? To deserialize, we would just slice of the first part, which would be a fixed length data for the timestamp and then deserialize the rest as data. I'm open to other ideas.

@hank121314
Copy link
Collaborator Author

hank121314 commented Feb 2, 2024

Maybe we can convert the timestamp to fixed-length data, serialize the value to data too, combine them, and store them both in one Data value?

I believe there might be some effort required in serializing the value into data.
Considering that the value sent to remote storage can be guaranteed as a UserDefaults-acceptable object (retrieved from local storage, which is UserDefaults), we can simplify the process by storing a two-length array.
The first element would be the timestamp, and the second element would be the value (ex. [Date(), 1]).

@sindresorhus
Copy link
Owner

The first element would be the timestamp, and the second element would be the value (ex. [Date(), 1]).

👍

@hank121314
Copy link
Collaborator Author

hank121314 commented Mar 3, 2024

PR updated!
Do following changes:

  1. The timestamp will now be based on the key to ensure accurate source detection.
  2. To ensure consistency between two storage's timestamp, each synchronization task will be assigned a unique timestamp with @TaskLocal.
  3. Will automatically sync the key after Defaults.iCloud.add

Please feel free to review it again! Many thanks!

@sindresorhus
Copy link
Owner

I finally had a chance to test this in a real app with syncing between Mac and iPhone and it seems to work fine 👍

@sindresorhus sindresorhus merged commit e837052 into sindresorhus:main Apr 20, 2024
2 checks passed
@sindresorhus
Copy link
Owner

Superb work @hank121314 🙌

@sindresorhus
Copy link
Owner

I missed that this was not resolved: https://github.com/sindresorhus/Defaults/pull/136/files#r1544553011

@sindresorhus
Copy link
Owner

sindresorhus commented Apr 20, 2024

I decided to make syncOnChange true by default (and internal) and renamed .sync() to .waitForSyncCompletion().

@hank121314
Copy link
Collaborator Author

I decided to make syncOnChange true by default (and internal) and renamed .sync() to .waitForSyncCompletion().

Sure, the new name looks more explicit about the behavior!

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.

Support NSUbiquitousKeyValueStore
3 participants