Repository navigation
[shared_preferences] switch to ffi/jni pigeon - #12329
Conversation
|
@stuartmorgan-g this is a working example of switching to ffi/jni pigeon. the pr contains all of the pigeon changes, but you can just look at the shared preferences stuff |
stuartmorgan-g
left a comment
stuartmorgan-g
left a comment
There was a problem hiding this comment.
The iOS part generally looks good, but one question on the binary file. The Android part I have more questions about the structure of main package vs example package.
stuartmorgan-g
left a comment
stuartmorgan-g
left a comment
There was a problem hiding this comment.
The Android structure is looking good now! One significant remaining issue for actually adopting this PR, but that's not related to the Pigeon generation itself, so in terms of validating the other PR I think we're in good shape here. I'll start on what it looks like should be the final review of the Pigeon PR shortly!
| @HostApi() | ||
| abstract class SharedPreferencesApi { | ||
| /// Removes property from shared preferences data set. | ||
| @TaskQueue(type: TaskQueueType.serialBackgroundThread) |
There was a problem hiding this comment.
All the implementations on the native side need to change to manually dispatch to a background thread (like they did before we had task queues, but presumably it's easier in Kotlin) so that we aren't regressing the old issue of doing file access on the main thread.
There was a problem hiding this comment.
I guess I should add task queue to the feature list for pigeon
There was a problem hiding this comment.
I was thinking about that a bit, and I wouldn't recommend it. Task queues were added specifically to address a pre-thread-merge problem where we had Dart code, which already wasn't running on the main thread, and it wanted to trigger background-thread tasks on the native side, but in order to do that everything was bouncing through the main thread because that's how message channels worked, so message channels themselves created main thread contention, and there was nothing clients could do about it. Task queues made the engine avoid the bounce through the main thread.
In an FFI world though, if code is on the main thread on the native side it's because it was being called from main thread Dart code. And so this is just regular programming, where if you want something not to be on the main thread its up to you to move it. We could build a bunch of machinery into Pigeon for that, but there's nothing specific to FFI or Pigeon about the problem, so I don't think it's probably a good idea to add the complexity there.
There was a problem hiding this comment.
could we dispatch this from an isolate on the dart side?
There was a problem hiding this comment.
I would talk to the Dart folks before adopting that approach here; IIRC there are still some significant foot-guns around trying to dispatch async FFI calls from a short-lived isolate.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request migrates the Android and iOS/macOS implementations of the shared_preferences plugin to use Pigeon's JNI and FFI generators, respectively. The changes include updating the threading model to use background dispatchers for asynchronous operations and refactoring plugin registration to improve lifecycle management. The review feedback highlights the need to explicitly manage the lifecycle of the LegacySharedPreferencesPlugin instance to prevent memory leaks and identifies a copy-paste error in a test case that requires correction.
| sdk: ^3.13.0 | ||
| flutter: ">=3.47.0" |
There was a problem hiding this comment.
This isn't ideal, but it is required with ffi pigeon
Piinks
left a comment
Piinks
left a comment
There was a problem hiding this comment.
Bumping the minimum Dart SDK constraint enables Dart 3.13 formatting rules. Running the format tool should resolve the CI failure.
| } | ||
| return value | ||
| } | ||
| override suspend fun getInt(key: String, options: SharedPreferencesPigeonOptions): Long? = |
There was a problem hiding this comment.
I am still wrapping my head around this, so feel free to tell me I am misunderstandning..
In DataStore Preferences, key equality only checks the key name, and preferences[preferencesKey] performs an unchecked generic cast that is erased to Object at runtime.
Previously, getInt, getBool, and getString were non-suspend functions that assigned preferenceFlow.firstOrNull() to a typed local variable inside runBlocking, which caused Kotlin to emit a CHECKCAST instruction and throw ClassCastException when the stored value had a different type.
Now that these methods are suspend functions returning withContext(backgroundDispatcher) { ... preferenceFlow.firstOrNull() }, all intermediate calls and the suspend continuation signature operate on erased Object at the bytecode level, so no cast is emitted in Kotlin (unlike getDouble on line 196, which has an explicit as Double? cast). When invoked from JNI, a type mismatch in DataStore completes the coroutine normally with the wrong Java object, causing the generated JNI bridge in messages_async.g.jni.dart to throw a JNI CastError (bypassing _convertKnownExceptions and leaking the global reference) instead of throwing a Dart TypeError.
Should we ensure the runtime cast happens on the Kotlin side in getInt, getBool, and getString, and add a test for reading a mismatched primitive type (e.g. calling getBool on a String key) in example/integration_test/shared_preferences_test.dart?
There was a problem hiding this comment.
Good catch! You are correct
tarrinneal
added
the
autosubmit
3 participants
Updates Android and Foundation implementations to use the latest pigeon with FFI and JNI. Should not change any underlying behaviors. Primarily to validate the latest pigeon functionality.