Conversation
92c97ac to
2c22caf
Compare
jwinarske
left a comment
There was a problem hiding this comment.
-
stop() blocks the calling Dart thread for up to ~1.5s per instance (blocking). The reader C ABI is synchronous — FlatpakBindings.readerStop runs the C++ inline on whatever thread calls it, and in a Flutter app that's the UI isolate's thread. The SIGTERM grace loop (15 × usleep(100000), serial across matched instances, plus SIGKILL escalation) means a "close app" tap freezes the homescreen for 1.5s+ whenever an app doesn't exit instantly. The existing reader calls get away with the synchronous pattern because they're fast enumerations; this one deliberately waits. Options: enqueue on a worker (the TransactionWorker serial-queue pattern is already in the codebase), or do SIGTERM immediately, post the sentinel, and run the grace/SIGKILL escalation via g_timeout_add on the bridge's GMainContext thread. The same argument applies to launch()'s installed-refs scan on cold cache, though that's less severe.
-
DO_NOT_REAP with no reaper leaks zombies (blocking). flatpak_installation_launch_full is called with FLATPAK_LAUNCH_FLAGS_DO_NOT_REAP, the returned FlatpakInstance is discarded, and nothing ever waitpids the bwrap pid. Every launched app leaves a zombie in the embedder process when it exits — for a homescreen that runs for months, that's unbounded. Note that dropping the flag doesn't fix it either: the default reap path installs a child watch on the thread-default main context, and launch() runs on the Dart thread where that's the global default context, which a plain Dart/Flutter-embedder process isn't iterating. The bridge already owns a dedicated GMainContext/GMainLoop thread (flatpak_bridge.cpp), so the clean fix is g_child_watch_add attached to that context with the pid from the returned instance, or a process-wide SIGCHLD reaper if ivi-homescreen doesn't already own one.
-
Launch result is thrown away. Since you have the FlatpakInstance in hand, post it as a 0x01 FpInstance payload before the sentinel and have launch() return Future. That eliminates the racy Future.delayed(1s) + listRunning() dance the example has to do to learn the instanceId, and it's the pid you need for finding #2 anyway. API change now is cheap; after merge it isn't.
-
Every error maps to FlatpakNotFoundException. Launch fails for reasons that aren't "not found" — missing runtime, sandbox setup failure, dbus-proxy spawn errors. Callers doing on FlatpakNotFoundException to mean "show install button" will misroute real failures. Add a FlatpakLaunchException (the exceptions.dart sealed hierarchy makes this trivial) or at least distinguish the "app not installed" pre-check from launch_full errors.
Smaller items:
- Branch/arch resolution asymmetry: if the caller supplies branch but not arch, the resolution loop filters on use_arch (null → no filter) but never checks use_branch, so resolved_arch can come from a ref of a different branch than the one requested. Filter on both when supplied. Also consider flatpak_installation_get_current_installed_app() for the no-hints path instead of scanning all refs — it respects flatpak's own "current" selection rather than first-match order, which matters when multiple branches are installed.
- stop() semantics: kill(pid, 0) returning EPERM means the process exists but isn't signallable; the code treats it as dead and skips. Within a session installation this is unlikely to matter, but errno != ESRCH should probably still count as found. Also the kill(child_pid, SIGTERM) return value is ignored — found stays true even if the signal failed. And unconditional SIGKILL escalation after 1.5s is a policy decision worth a doc comment or a parameter; the C++ plugin's ApplicationStop didn't escalate.
- instance_test.dart: the "launch is a method on FlatpakClient" test asserts FlatpakClient.system isA and nothing about launch — it's a no-op. Either drop it or keep only the model tests, which are fine.
- transaction_bridge.cpp G_CONNECT_DEFAULT → static_cast(0): fine (drops the GLib ≥ 2.74 requirement, presumably for the Yocto/older-distro build), but it's an unrelated drive-by in a commit titled "add flatpak apps lifecycle" — worth a line in the commit message so it doesn't look accidental.
- Codec: field order in decodeInstance matches the glz::meta tuple, and the roundtrip test covers the C++ side. No cross-language fixture, but that matches the existing structs, so no new debt.
- CI: pull-requests: write + continue-on-error on the lcov comment step is the right call for fork PRs.
Just need 1-4
|
Coverage after merging launcher into main will be
Coverage Report for Changed Files
|
|||||||||||||||||||||||||||||||||||||
07a4890 to
8e47b9f
Compare
Correct and worth keeping
Issues, by severity1.
|
cf03a5e to
3647072
Compare
|
Coverage after merging launcher into main will be
Coverage Report for Changed Files
|
|||||||||||||||||||||||||||||||||||||
Previous findings addressedThis commit resolves four of the five round-1 findings:
Thread-safety premise verified: upstream Resolution logic traced: New issues introduced by this commit1. Destructor drops queued launches, hanging Dart futures
Fix: drain instead of stop — 2.
|
|
Coverage after merging launcher into main will be
Coverage Report for Changed Files
|
|||||||||||||||||||||||||||||||||||||
|
@AhmedAdelWafdy7 Needs a rebase. Baseline package published: https://pub.dev/packages/flatpak_dart |
…pakClient and Installation classes with improved error handling and instance management
…e signal handling during app termination
c171bfe to
ef5623d
Compare
|
Coverage after merging launcher into main will be
Coverage Report for Changed Files
|
|||||||||||||||||||
|
Coverage after merging launcher into main will be
Coverage Report for Changed Files
|
|||||||||||||||||||
|
@jwinarske, the rebase is complete! |
|
Pushed four follow-up commits ( asio is gone — no downstream dependency neededThe review flagged that This means the Review itemsAll "must fix" and "should fix" items are addressed, plus the nits. Header doc drift corrected ( Two things surfaced while testing that went beyond the review:
Also hardened: VerificationASan/UBSan 4/4 · TSAN clean of our code (the 9 reports are GLib/GDBus internals — |
|
Coverage after merging launcher into main will be
Coverage Report for Changed Files
|
|||||||||||||||||||
Drop asio in favour of the serial-queue pattern already used by TransactionWorker (std::thread + mutex + condvar + queue). asio was pulling in a system dependency for a single one-op queue that had to be satisfied by four CI distros and every consumer sysroot, and it was never wired into native/CMakeLists.txt — configure succeeded and compile failed cold for anyone building from source. Removing it also means no `DEPENDS += asio` is needed in the meta-flutter recipe. Destruction order is preserved: drain the queue, join, then unref, so a launch already queued still runs against a live installation_. Fix header doc drift in flatpak_bridge.h: launch uses launch_full() with DO_NOT_REAP and posts 0x01 FpInstance + 0xFF on success, 0x02 for not-installed and 0x03 for launch failure; stop signals the sandboxed app process (child pid), not the outer bwrap pid. Also: - TODO on reap_async: one parked thread per running app should become a single epoll'd reaper multiplexing pidfds. - Document that stop() is host-wide (flatpak instances are not scoped to an installation) on both FlatpakClient.stop and Installation.stop. - Comment the non-pidfd kill() fallback as intentionally un-escalated. - Replace the no-op "launch is a method" test with real compile-time signature checks, and pin the FpInstance wire format with a golden buffer emitted by the C++ writer and decoded by GlazeCodec. - Correct the grace period in example/launch_app.dart (1.5s, not 2s). - Note the defensive 0x02 branch in listRunning. - Restore the section divider rule style in installation_reader.cpp. Signed-off-by: Joel Winarske <joel.winarske@linux.com>
flatpak_instance_get_child_pid() returns whatever the instance directory held when the FlatpakInstance was constructed, and launch_full() builds that object before bwrap has written the pid file — so it always read 0, which made the launch dartdoc's promise that callers get the instance details without polling listRunning() true only for instanceId and pid. Re-enumerate flatpak_instance_get_all() on the launch thread, matching the instance id, until a freshly constructed object carries the real pid. libflatpak 1.18.1 exposes no flatpak_instance_new_for_id(), so get_all() is the only way to force the re-read. Bounded at 500ms in 5ms polls, and bails early once the instance has appeared and then vanished, so an app that exits before bwrap publishes the pid cannot park the launch thread. Timing out still yields 0, which is what callers saw before, so this is strictly an improvement rather than a new failure mode. Measured cost on the normal path is nil: launch() round-trips in ~55ms with or without the re-read. Document childPid as best-effort on FlatpakClient.launch, Installation.launch and the FlatpakInstance.childPid field. Verified end to end against org.gnome.Calculator: launch() now returns childPid=3453764, matching what listRunning() reports for the same instance id. Signed-off-by: Joel Winarske <joel.winarske@linux.com>
Addresses the reliability, security and performance findings from review of the launcher branch. Reliability: R1 stop() no longer reports "no running instance" for an app that is running. child_pid <= 0 previously dropped the instance while `found` stayed false, so the caller got FlatpakNotFoundException and the app was never signalled. Now falls back to the outer bwrap pid, and tracks matched separately from signalled so "matched but could not signal" reports as 0x03 / FlatpakStopException instead of a not-found. R2 reap_thread() retries waitpid() on EINTR. A single unrestarted call would abandon the bwrap child as a zombie for the lifetime of the host process — the exact leak DO_NOT_REAP + the reaper exist to prevent. The Dart VM delivers SIGPROF; embedders install their own handlers. R3 resolve_launch_target() propagates the list_installed_refs GError rather than discarding it. An unreadable installation reported as "app not installed" sends callers after the wrong problem. R4 The no-pidfd fallback now escalates to SIGKILL. Measured: GTK apps ignore SIGTERM and exit only on the escalation, so a TERM-only fallback would silently fail to stop them on a pre-5.3 kernel. R5 Documented why installation_ needs no mutex (libflatpak documents FlatpakInstallation as concurrency-safe) now that launch_impl touches it from the launch thread. The invariant was previously an undocumented assumption after the single-thread comment was dropped. Security: S1 stop() skips instances whose process has already exited, and verifies process identity via /proc/<pid>/stat field 22 (start time) before signalling. pidfd pins a process only after the open; the pid came off disk and could have been recycled in between, which would have sent SIGTERM and then SIGKILL to an unrelated process the user owns. The fallback path re-checks identity before the kill for the same reason. Performance: P1 reread_child_pid() backs off geometrically (1,2,4..64ms) instead of polling every 5ms. Each probe re-parses the info file of every running flatpak on the host; this covers the same 500ms in ~13 probes rather than ~100, with the common path (found on the first probe) unchanged. P2 Shutdown cancels the queued launch backlog instead of draining it, so close() costs at most the in-flight launch. Each cancelled request still gets an error frame, so no Dart future hangs. Measured: close() with 5 queued went from ~5 sequential sandbox spawns to 53ms, with 1 launched and 4 cancelled cleanly. P3 Documented why stop()/list_running() stay on the Dart thread rather than joining the launch queue: measured at ~102us (3 instances) to ~161us (9) for list_running and ~727us for a stop matching 6, well inside a frame, and queueing them would make stop() wait on an in-flight sandbox spawn. Verified: ASan/UBSan 4/4, TSAN clean of our code (9 GLib/GDBus reports, same baseline as main), 72/72 Dart, -Wall -Wextra clean, launch latency unchanged at ~47-64ms, and stop() latency A/B'd against the previous commit (1.6-2.0s settled, ~4s mid-startup) — identical either side. Signed-off-by: Joel Winarske <joel.winarske@linux.com>
bugprone-implicit-widening-of-multiplication-result: both sleeps computed int * int and let the result widen to g_usleep's gulong parameter. Values are small enough that this could not overflow in practice, but the cast makes the arithmetic happen in the destination type. clang-tidy-19 is now clean across all four native sources with the project's .clang-tidy config, and clang-format reports every file correctly formatted. Signed-off-by: Joel Winarske <joel.winarske@linux.com>
|
Coverage after merging launcher into main will be
Coverage Report for Changed Files
|
|||||||||||||||||||
Adds the first piece of IVI-launcher support to
flatpak_dart: launching an installed app ("tap to open"), stopping it, and listing running instances. Ported from the ivi-homescreen C++ plugin'sApplicationStart/ApplicationStop/find_running_instance.added flutter examples :
I tested the package on a Flutter ivi app.

