From c59205d07993a853470be42eaba8f5f1ecfdefe9 Mon Sep 17 00:00:00 2001 From: Eric Seidel Date: Wed, 8 Mar 2023 13:00:04 -0800 Subject: [PATCH] Make updater library thread safe This was needed so that it could be called from Dart as well as flutter_main/C++. It turns out flutter_main does not run on the "ui thread", so when Dart was calling into the updater it would panic due to thinking the updater (which was using a thread local) was not yet initialized. Also added the log-panics crate on Android so that panics appear in adb logcat. --- example/counter/lib/main.dart | 3 +- example/counter/pubspec.lock | 2 +- updater/cbindgen.toml | 13 +++ updater/dart_bindings/lib/src/bindings.dart | 29 ++++--- updater/library/Cargo.toml | 4 +- updater/library/README.md | 56 +++++++++++++ updater/library/include/updater.h | 18 ++-- updater/library/src/config.rs | 93 ++++++++++++--------- updater/library/src/logging.rs | 7 +- 9 files changed, 162 insertions(+), 63 deletions(-) diff --git a/example/counter/lib/main.dart b/example/counter/lib/main.dart index fe5cbc18..8eceba50 100644 --- a/example/counter/lib/main.dart +++ b/example/counter/lib/main.dart @@ -5,7 +5,8 @@ void main() { Updater.loadFlutterLibrary(); var updater = Updater(); // Just to prove the bindings work at all: - print(updater.activeVersion()); + print("active version: ${updater.activeVersion()}"); + print("active path: ${updater.activePath()}"); runApp(const MyApp()); } diff --git a/example/counter/pubspec.lock b/example/counter/pubspec.lock index 32dd9632..fc2aef9d 100644 --- a/example/counter/pubspec.lock +++ b/example/counter/pubspec.lock @@ -200,4 +200,4 @@ packages: source: hosted version: "2.1.4" sdks: - dart: ">=3.0.0-266.0.dev <4.0.0" + dart: ">=2.19.0 <4.0.0" diff --git a/updater/cbindgen.toml b/updater/cbindgen.toml index d7624ad1..aa67c5a4 100644 --- a/updater/cbindgen.toml +++ b/updater/cbindgen.toml @@ -5,3 +5,16 @@ include_guard = "updater_h" autogen_warning = "/* Warning, this file is autogenerated by cbindgen. Don't modify this manually. */" cpp_compat = true line_length = 80 + +# I don't know if these are required to export the shorebird_ symbols +# since I've hit multiple levels of export trouble in libflutter.so. +# But I'm leaving them here for now. +after_includes = """ +#ifdef _WIN32 +#define SHOREBIRD_EXPORT __declspec(dllexport) +#else +#define SHOREBIRD_EXPORT __attribute__((visibility("default"))) +#endif +""" +[fn] +prefix = "SHOREBIRD_EXPORT" \ No newline at end of file diff --git a/updater/dart_bindings/lib/src/bindings.dart b/updater/dart_bindings/lib/src/bindings.dart index 2b74b457..8651b5d9 100644 --- a/updater/dart_bindings/lib/src/bindings.dart +++ b/updater/dart_bindings/lib/src/bindings.dart @@ -2,10 +2,8 @@ // Probably https://pub.dev/packages/ffigen would work. import 'dart:ffi' as ffi; -import 'dart:io' show Directory, Platform; import 'package:ffi/ffi.dart'; -import 'package:path/path.dart' as path; // This must be kept in sync with the C struct in updater.h. // Including *in the same order* as the C struct. @@ -88,15 +86,22 @@ class UpdaterBindings { late GetVoid update; UpdaterBindings(this.library) { - init = library.lookupFunction<_SBInitFunc, SBInit>('shorebird_init'); - activeVersion = library - .lookupFunction<_GetStringFunc, GetString>('shorebird_active_version'); - activePath = library - .lookupFunction<_GetStringFunc, GetString>('shorebird_active_path'); - freeString = library - .lookupFunction<_FreeStringFunc, FreeString>('shorebird_free_string'); - checkForUpdate = library - .lookupFunction<_GetBoolFunc, GetBool>('shorebird_check_for_update'); - update = library.lookupFunction<_GetVoidFunc, GetVoid>('shorebird_update'); + // None of these call back into Dart, so they're all safely "isLeaf: true". + init = library.lookupFunction<_SBInitFunc, SBInit>('shorebird_init', + isLeaf: true); + activeVersion = library.lookupFunction<_GetStringFunc, GetString>( + 'shorebird_active_version', + isLeaf: true); + activePath = library.lookupFunction<_GetStringFunc, GetString>( + 'shorebird_active_path', + isLeaf: true); + freeString = library.lookupFunction<_FreeStringFunc, FreeString>( + 'shorebird_free_string', + isLeaf: true); + checkForUpdate = library.lookupFunction<_GetBoolFunc, GetBool>( + 'shorebird_check_for_update', + isLeaf: true); + update = library.lookupFunction<_GetVoidFunc, GetVoid>('shorebird_update', + isLeaf: true); } } diff --git a/updater/library/Cargo.toml b/updater/library/Cargo.toml index c73f7f54..df4b398b 100644 --- a/updater/library/Cargo.toml +++ b/updater/library/Cargo.toml @@ -21,8 +21,10 @@ serde = { version = "1.0", features = ["derive"] } serde_json = "1.0.93" # Used for error handling. anyhow = {version = "1.0.69", features = ["backtrace"]} -# For error!(), info!(), etc macros. +# For error!(), info!(), etc macros. `print` will not show up on Android. log = "0.4.14" +once_cell = "1.17.1" [target.'cfg(target_os = "android")'.dependencies] android_logger = "0.13.0" +log-panics = { version = "2", features = ["with-backtrace"]} diff --git a/updater/library/README.md b/updater/library/README.md index 1ca6af4c..7228a3f4 100644 --- a/updater/library/README.md +++ b/updater/library/README.md @@ -2,6 +2,61 @@ The rust library that does the actual update work. +## Design + +The updater library is built in Rust for safety (and modernity). It's built +as a C-compatible library, so it can be used from any language. + +The library is thread-safe, as it needs to be called both from the flutter_main +thread (during initialization) and then later from the Dart/UI thread +(from application Dart code) in Flutter. + +The overarching principle with the Updater is "first, do no harm". The updater +should "fail open", terms of continuing to work with the currently installed +or active version of the application even when the network is unavailable. + +The updater also needs to handle error cases conservatively, such as partial +downloads from a server, or malformed responses (e.g. a proxy interfering) +and not crash the application or leave the application in a broken state. + +Every time the updater runs it needs to verify that the currently installed +patch is compatible with the currently installed base version. If it is not, +it should refuse to return paths to incompatible patches. + +The updater also needs to regularly verify that the current state directory +is in a consistent state. If it is not, it should invalidate any installed +patches and return to a clean state. + +Not all of the above is implemented yet, but such is the intent. + +## Architecture + +The updater is split into separate layers. The top layer is the C-compatible +API, which is used by all consumers of the updater. The C-compatible API +is a thin wrapper around the Rust API, which is the main implementation but +only used directly for testing (see the `cli` directory). + +Thread safety is handled by a global configuration object that is locked +when accessed. It's possible I've missed cases where this is not sufficient, +and there could be thread safety issues in the library. + +* src/c_api.rs - C-compatible API +* src/lib.rs - Rust API (and crate root) +* src/update.rs - Core updater logic +* src/config.rs - In memory configuration and thread locking +* src/cache.rs - On-disk state management +* src/logging.rs - Logging configuration (for platforms that need it) +* src/network.rs - Logic dealing with network requests and updater server + +## Integration + +The updater library is built as a static library, and is linked into the +libflutter.so as part of a custom build of Flutter. We also link libflutter.so +with the correct flags such that updater symbols are exposed to Dart. + +The `dart_bindings` directory contains the Dart bindings for the updater +library. + ## Building for Android The best way I found was to install: @@ -18,6 +73,7 @@ rustup +beta target add \ cargo +beta ndk --target aarch64-linux-android build --release ``` +## Development Uses cbindgen to generate the header file. diff --git a/updater/library/include/updater.h b/updater/library/include/updater.h index 3f9089a5..a308c0a4 100644 --- a/updater/library/include/updater.h +++ b/updater/library/include/updater.h @@ -7,6 +7,12 @@ #include #include #include +#ifdef _WIN32 +#define SHOREBIRD_EXPORT __declspec(dllexport) +#else +#define SHOREBIRD_EXPORT __attribute__((visibility("default"))) +#endif + /** * Struct containing configuration parameters for the updater. @@ -61,33 +67,33 @@ typedef struct AppParameters { extern "C" { #endif // __cplusplus -void shorebird_init(const struct AppParameters *c_params); +SHOREBIRD_EXPORT void shorebird_init(const struct AppParameters *c_params); /** * Return the active version of the app, or NULL if there is no active version. */ -char *shorebird_active_version(void); +SHOREBIRD_EXPORT char *shorebird_active_version(void); /** * Return the path to the active version of the app, or NULL if there is no * active version. */ -char *shorebird_active_path(void); +SHOREBIRD_EXPORT char *shorebird_active_path(void); /** * Free a string returned by the updater library. */ -void shorebird_free_string(char *c_string); +SHOREBIRD_EXPORT void shorebird_free_string(char *c_string); /** * Check for an update. Returns true if an update is available. */ -bool shorebird_check_for_update(void); +SHOREBIRD_EXPORT bool shorebird_check_for_update(void); /** * Synchronously download an update if one is available. */ -void shorebird_update(void); +SHOREBIRD_EXPORT void shorebird_update(void); #ifdef __cplusplus } // extern "C" diff --git a/updater/library/src/config.rs b/updater/library/src/config.rs index c5f763d0..f8d89200 100644 --- a/updater/library/src/config.rs +++ b/updater/library/src/config.rs @@ -1,38 +1,38 @@ // This file handles the global config for the updater library. -use std::cell::RefCell; +use std::sync::Mutex; use crate::updater::AppConfig; +use once_cell::sync::OnceCell; +// cbindgen looks for const, ignore these so it doesn't warn about them. + +/// cbindgen:ignore const DEFAULT_BASE_URL: &'static str = "https://shorebird-code-push-api-cypqazu4da-uc.a.run.app"; +/// cbindgen:ignore const DEFAULT_CHANNEL: &'static str = "stable"; -thread_local!(static CONFIG: RefCell> = RefCell::new(None)); +fn global_config() -> &'static Mutex { + static INSTANCE: OnceCell> = OnceCell::new(); + INSTANCE.get_or_init(|| Mutex::new(ResolvedConfig::empty())) +} pub fn with_config(f: F) -> R where F: FnOnce(&ResolvedConfig) -> R, { - CONFIG - .try_with(|config| { - let config = config.borrow(); - let config = config - .as_ref() - .expect("Must call updater_init before using the updater library."); - return f(config); - }) - .expect("Must call updater_init before using the updater library.") -} + let lock = global_config() + .lock() + .expect("Failed to acquire updater lock."); -pub fn set_config(config: AppConfig) { - let config = resolve_config(config); - CONFIG.with(|c| { - let mut c = c.borrow_mut(); - *c = Some(config); - }); + if !lock.is_initialized { + panic!("Must call shorebird_init() before using the updater."); + } + return f(&lock); } pub struct ResolvedConfig { + is_initialized: bool, pub cache_dir: String, pub channel: String, pub client_id: String, @@ -43,26 +43,43 @@ pub struct ResolvedConfig { pub base_url: String, } -fn resolve_config(config: AppConfig) -> ResolvedConfig { - // Resolve the config +impl ResolvedConfig { + pub fn empty() -> Self { + Self { + is_initialized: false, + cache_dir: String::new(), + channel: String::new(), + client_id: String::new(), + product_id: String::new(), + base_version: String::new(), + original_libapp_path: String::new(), + vm_path: String::new(), + base_url: String::new(), + } + } +} + +pub fn set_config(config: AppConfig) { // If there is no base_url, use the default. // If there is no channel, use the default. - return ResolvedConfig { - client_id: config.client_id.to_string(), - base_url: config - .base_url - .as_deref() - .unwrap_or(DEFAULT_BASE_URL) - .to_owned(), - cache_dir: config.cache_dir.to_string(), - channel: config - .channel - .as_deref() - .unwrap_or(DEFAULT_CHANNEL) - .to_owned(), - product_id: config.product_id.to_string(), - base_version: config.base_version.to_string(), - original_libapp_path: config.original_libapp_path.to_string(), - vm_path: config.vm_path.to_string(), - }; + let mut lock = global_config() + .lock() + .expect("Failed to acquire updater lock."); + lock.base_url = config + .base_url + .as_deref() + .unwrap_or(DEFAULT_BASE_URL) + .to_owned(); + lock.channel = config + .channel + .as_deref() + .unwrap_or(DEFAULT_CHANNEL) + .to_owned(); + lock.cache_dir = config.cache_dir.to_string(); + lock.client_id = config.client_id.to_string(); + lock.product_id = config.product_id.to_string(); + lock.base_version = config.base_version.to_string(); + lock.original_libapp_path = config.original_libapp_path.to_string(); + lock.vm_path = config.vm_path.to_string(); + lock.is_initialized = true; } diff --git a/updater/library/src/logging.rs b/updater/library/src/logging.rs index 1616a2c7..b4cc6d81 100644 --- a/updater/library/src/logging.rs +++ b/updater/library/src/logging.rs @@ -1,13 +1,12 @@ #[cfg(target_os = "android")] pub fn init_logging() { - use android_logger::Config; - use log::LevelFilter; + log_panics::init(); android_logger::init_once( - Config::default() + android_logger::Config::default() // `flutter` tool ignores non-flutter tagged logs. .with_tag("flutter") - .with_max_level(LevelFilter::Debug), + .with_max_level(log::LevelFilter::Debug), ); debug!("Logging initialized"); }