mirror of
https://github.com/narrrl/proton-drive-linux.git
synced 2026-11-03 16:41:26 +01:00
fix(gui,core): ask the mount off the main thread when opening, reap children
Opening a file or a location stat'ed the mount on the GTK main thread, so a hung mount hung the app. The browser now uses the launcher's bounded worker check (`present_within`, 2 s), and opening a location runs on a thread. Every child the GUI starts is also waited for on a thread, so it no longer stays a zombie (B243).
This commit is contained in:
1 parent
6ec8043934
commit
48c8910e7b
8 files changed
+137
-72
No files matched your search
@@ -189,11 +189,21 @@ fn spawn(argv: &[String], path: &Path) {
|
||||
.stderr(Stdio::null())
|
||||
.process_group(0)
|
||||
.spawn();
|
||||
if let Err(e) = spawned {
|
||||
tracing::error!(command = ?argv, "open failed: {e}");
|
||||
match spawned {
|
||||
Ok(child) => reap(child),
|
||||
Err(e) => tracing::error!(command = ?argv, "open failed: {e}"),
|
||||
}
|
||||
}
|
||||
|
||||
/// Wait for `child` on a thread of its own. A child nobody waits for stays a
|
||||
/// zombie for as long as its parent lives, which for the app and the tray is
|
||||
/// the whole session.
|
||||
pub fn reap(mut child: std::process::Child) {
|
||||
std::thread::spawn(move || {
|
||||
let _ = child.wait();
|
||||
});
|
||||
}
|
||||
|
||||
/// [`open`] with the policy from the user's `config.json`, read once per
|
||||
/// process. For callers that open the occasional path and have no reason to
|
||||
/// carry a policy around; long-lived windows should hold their own copy.
|
||||
@@ -472,6 +482,21 @@ fn glob_match(pattern: &str, name: &str) -> bool {
|
||||
mod tests {
|
||||
use super::*;
|
||||
|
||||
#[test]
|
||||
fn a_reaped_child_does_not_stay_a_zombie() {
|
||||
let child = std::process::Command::new("true").spawn().unwrap();
|
||||
let proc_entry = std::path::PathBuf::from(format!("/proc/{}", child.id()));
|
||||
reap(child);
|
||||
let deadline = std::time::Instant::now() + std::time::Duration::from_secs(5);
|
||||
while proc_entry.exists() && std::time::Instant::now() < deadline {
|
||||
std::thread::sleep(std::time::Duration::from_millis(10));
|
||||
}
|
||||
assert!(
|
||||
!proc_entry.exists(),
|
||||
"the child is still in the process table"
|
||||
);
|
||||
}
|
||||
|
||||
fn rule(patterns: &[&str], command: &[&str], terminal: bool) -> OpenRule {
|
||||
OpenRule {
|
||||
patterns: patterns.iter().map(|p| (*p).to_string()).collect(),
|
||||
|
||||
@@ -10,9 +10,14 @@
|
||||
//! `dead_code` allowances below are for — not code nobody uses.
|
||||
|
||||
use std::path::{Path, PathBuf};
|
||||
use std::time::Duration;
|
||||
|
||||
use pdfs_core::control::{PhotoKind, SearchHit};
|
||||
|
||||
/// How long a `stat` of the mount may take before the file counts as not
|
||||
/// there. A frozen mount never answers, and every caller has a fallback.
|
||||
const STAT_TIMEOUT: Duration = Duration::from_secs(2);
|
||||
|
||||
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
|
||||
pub(crate) enum DriveActivation {
|
||||
Folder,
|
||||
@@ -55,18 +60,32 @@ pub(crate) fn mounted_target(mountpoint: &Path, hit: &SearchHit) -> Option<PathB
|
||||
present(mounted_or_relative(mountpoint, hit))
|
||||
}
|
||||
|
||||
/// [`mounted_target`] for callers holding a mountpoint-relative path rather
|
||||
/// than a search hit — the browser's listing entries, which are by definition
|
||||
/// paths in the primary mount's tree.
|
||||
#[allow(dead_code)] // browser only
|
||||
pub(crate) fn mounted_target_rel(mountpoint: &Path, relative_path: &str) -> Option<PathBuf> {
|
||||
present(mounted_path(mountpoint, relative_path))
|
||||
}
|
||||
|
||||
fn present(path: PathBuf) -> Option<PathBuf> {
|
||||
path.symlink_metadata().is_ok().then_some(path)
|
||||
}
|
||||
|
||||
/// Whether anything is at `path`, asked on a thread of its own and given up
|
||||
/// on after [`STAT_TIMEOUT`]. `None` means "not there" or "no answer".
|
||||
///
|
||||
/// For the GTK binaries, whose main thread must not `stat` the mount: a
|
||||
/// mount that hangs hung the whole window with it. The thread of a `stat`
|
||||
/// that never returns is left behind; there is no way to cancel one.
|
||||
#[allow(dead_code)] // GTK binaries only
|
||||
pub(crate) async fn present_within(path: PathBuf) -> Option<PathBuf> {
|
||||
let (tx, rx) = async_channel::bounded(1);
|
||||
std::thread::spawn(move || {
|
||||
let _ = tx.send_blocking(present(path));
|
||||
});
|
||||
match gtk4::glib::future_with_timeout(STAT_TIMEOUT, rx.recv()).await {
|
||||
Ok(Ok(target)) => target,
|
||||
Ok(Err(_)) => None,
|
||||
Err(_) => {
|
||||
tracing::warn!("the mount did not answer in time");
|
||||
None
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// The mount path for a hit, whether or not anything is there — for callers
|
||||
/// that have no fallback (a folder cannot be materialised) or already know the
|
||||
/// entry came from a mount listing.
|
||||
|
||||
@@ -297,7 +297,10 @@ fn present_window(app: &adw::Application) {
|
||||
/// Spawn the tray icon process in the background.
|
||||
pub(crate) fn spawn_tray() {
|
||||
match Command::new("pdfs-tray").spawn() {
|
||||
Ok(_) => tracing::info!("spawned `pdfs-tray`"),
|
||||
Ok(child) => {
|
||||
tracing::info!("spawned `pdfs-tray`");
|
||||
pdfs_core::opener::reap(child);
|
||||
}
|
||||
Err(e) => tracing::error!("failed to spawn `pdfs-tray`: {e}"),
|
||||
}
|
||||
}
|
||||
@@ -1713,9 +1716,12 @@ pub(crate) fn flash_copied(button: >k4::Button, icon: &'static str) {
|
||||
|
||||
/// Open a local path with the handler the user configured — `xdg-open` unless
|
||||
/// `open_with` in `config.json` overrides it for this kind of file.
|
||||
///
|
||||
/// On a thread of its own: telling a folder from a file is a `stat`, and the
|
||||
/// path is often in the mount, which may hang.
|
||||
fn open_path(path: &str) {
|
||||
let path = Path::new(path);
|
||||
pdfs_core::opener::open_default(path, path.is_dir());
|
||||
let path = PathBuf::from(path);
|
||||
std::thread::spawn(move || pdfs_core::opener::open_default(&path, path.is_dir()));
|
||||
}
|
||||
|
||||
/// [`open_path`] for a file the daemon materialised into the content cache,
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
use crate::activation::{DriveActivation, drive_activation, mounted_path, mounted_target_rel};
|
||||
use crate::activation::{DriveActivation, drive_activation, mounted_path, present_within};
|
||||
use crate::*;
|
||||
|
||||
pub(crate) struct BrowserState {
|
||||
@@ -983,28 +983,27 @@ pub(crate) fn background_context_menu(ui: &Rc<Ui>) -> ActionMenu {
|
||||
pub(crate) fn open_entry_with(ui: &Rc<Ui>, entry: &DirEntry) {
|
||||
let rel = entry_rel(ui, entry);
|
||||
let mountpoint = ui.dirs.resolved_mountpoint(&ui.dirs.load_config());
|
||||
let Some(path) = mounted_target_rel(&mountpoint, &rel) else {
|
||||
toast_error(
|
||||
ui,
|
||||
&gettext("Couldn't open file"),
|
||||
&gettext("Open With needs the Proton Drive service to be running."),
|
||||
);
|
||||
return;
|
||||
};
|
||||
let launcher = gtk4::FileLauncher::new(Some(&gio::File::for_path(path)));
|
||||
launcher.set_always_ask(true);
|
||||
let ui = ui.clone();
|
||||
launcher.launch(
|
||||
ui_window(&ui).as_ref(),
|
||||
gio::Cancellable::NONE,
|
||||
move |result| {
|
||||
glib::spawn_future_local(async move {
|
||||
let Some(path) = present_within(mounted_path(&mountpoint, &rel)).await else {
|
||||
toast_error(
|
||||
&ui,
|
||||
&gettext("Couldn't open file"),
|
||||
&gettext("Open With needs the Proton Drive service to be running."),
|
||||
);
|
||||
return;
|
||||
};
|
||||
let launcher = gtk4::FileLauncher::new(Some(&gio::File::for_path(path)));
|
||||
launcher.set_always_ask(true);
|
||||
let window = ui_window(&ui);
|
||||
launcher.launch(window.as_ref(), gio::Cancellable::NONE, move |result| {
|
||||
if let Err(e) = result
|
||||
&& !e.matches(gtk4::DialogError::Dismissed)
|
||||
{
|
||||
toast_error(&ui, &gettext("Couldn't open file"), &e.to_string());
|
||||
}
|
||||
},
|
||||
);
|
||||
});
|
||||
});
|
||||
}
|
||||
|
||||
/// Copy the entry's public link, when it has one this client can read the
|
||||
@@ -1112,27 +1111,29 @@ pub(crate) fn activate_entry(ui: &Rc<Ui>, entry: &DirEntry) {
|
||||
/// [`mounted_target`](crate::activation::mounted_target).
|
||||
pub(crate) fn download_and_open(ui: &Rc<Ui>, entry: &DirEntry) {
|
||||
let rel = entry_rel(ui, entry);
|
||||
let mountpoint = ui.dirs.resolved_mountpoint(&ui.dirs.load_config());
|
||||
if let Some(path) = mounted_target_rel(&mountpoint, &rel) {
|
||||
open_path(&path.to_string_lossy());
|
||||
return;
|
||||
}
|
||||
// Ignore a repeat activation of a file already downloading, so an impatient
|
||||
// double-click doesn't kick off a second round-trip.
|
||||
// Ignore a repeat activation of a file already opening, so an impatient
|
||||
// double-click doesn't open it twice or kick off a second round-trip.
|
||||
if !ui.opening.borrow_mut().insert(rel.clone()) {
|
||||
return;
|
||||
}
|
||||
ui.busy_begin();
|
||||
let name = entry.name.clone();
|
||||
let rx = spawn_request(
|
||||
ui.dirs.control_socket(),
|
||||
Request::OpenFile {
|
||||
path: rel.clone(),
|
||||
uid: Some(entry.uid.clone()),
|
||||
},
|
||||
);
|
||||
let mountpoint = ui.dirs.resolved_mountpoint(&ui.dirs.load_config());
|
||||
let (name, uid) = (entry.name.clone(), entry.uid.clone());
|
||||
let ui = ui.clone();
|
||||
glib::spawn_future_local(async move {
|
||||
// Asked off the main thread: a mount that hangs used to hang the app.
|
||||
if let Some(path) = present_within(mounted_path(&mountpoint, &rel)).await {
|
||||
ui.opening.borrow_mut().remove(&rel);
|
||||
open_path(&path.to_string_lossy());
|
||||
return;
|
||||
}
|
||||
ui.busy_begin();
|
||||
let rx = spawn_request(
|
||||
ui.dirs.control_socket(),
|
||||
Request::OpenFile {
|
||||
path: rel.clone(),
|
||||
uid: Some(uid),
|
||||
},
|
||||
);
|
||||
let result = rx.recv().await;
|
||||
ui.busy_end();
|
||||
ui.opening.borrow_mut().remove(&rel);
|
||||
|
||||
@@ -3481,10 +3481,10 @@ pub(crate) fn gallery_unreachable(ui: &Rc<Ui>) {
|
||||
/// right tool for the HEVC `.mkv`s this is aimed at — and falls back to the
|
||||
/// user's default handler when mpv isn't installed.
|
||||
pub(crate) fn play_external(path: &str) {
|
||||
if Command::new("mpv").arg(path).spawn().is_ok() {
|
||||
return;
|
||||
match Command::new("mpv").arg(path).spawn() {
|
||||
Ok(child) => pdfs_core::opener::reap(child),
|
||||
Err(_) => open_path(path),
|
||||
}
|
||||
open_path(path);
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
|
||||
@@ -15,7 +15,6 @@ mod thumbs;
|
||||
use std::cell::{Cell, RefCell};
|
||||
use std::path::{Path, PathBuf};
|
||||
use std::rc::Rc;
|
||||
use std::time::Duration;
|
||||
|
||||
use adw::prelude::*;
|
||||
use gtk4::{gdk, gio, glib};
|
||||
@@ -24,7 +23,7 @@ use pdfs_core::config::AppDirs;
|
||||
use pdfs_core::control::{Request, Response, SearchFilters, SearchHit, SearchSource};
|
||||
use pdfs_core::opener::{self, OpenWith};
|
||||
|
||||
use crate::activation::{DriveActivation, drive_activation, mounted_or_relative, mounted_target};
|
||||
use crate::activation::{DriveActivation, drive_activation, mounted_or_relative, present_within};
|
||||
use crate::compat::{Spinner, spinner};
|
||||
use crate::theme::set_proton_theme;
|
||||
use crate::{
|
||||
@@ -38,10 +37,6 @@ use thumbs::Thumbs;
|
||||
/// Recent files shown above the pinned ones while the query is empty.
|
||||
const RECENT_LIMIT: usize = 8;
|
||||
|
||||
/// How long opening waits for the mount to answer whether it has a file. A
|
||||
/// frozen mount must not freeze the launcher; the daemon is asked instead.
|
||||
const STAT_TIMEOUT: Duration = Duration::from_secs(2);
|
||||
|
||||
/// The titled groups of the result list.
|
||||
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
|
||||
enum Section {
|
||||
@@ -516,22 +511,11 @@ pub(crate) fn build_window(app: &adw::Application) -> Option<Rc<Ui>> {
|
||||
Some(ui)
|
||||
}
|
||||
|
||||
/// Whether the mount has `hit`, asked on a worker thread and given up on
|
||||
/// after [`STAT_TIMEOUT`]. `None` means "not there" or "no answer"; either
|
||||
/// way the daemon is the fallback.
|
||||
/// Whether the mount has `hit`, without blocking the window
|
||||
/// ([`present_within`]). `None` means "not there" or "no answer"; either way
|
||||
/// the daemon is the fallback.
|
||||
async fn mounted_target_within(mountpoint: PathBuf, hit: SearchHit) -> Option<PathBuf> {
|
||||
let (tx, rx) = async_channel::bounded(1);
|
||||
std::thread::spawn(move || {
|
||||
let _ = tx.send_blocking(mounted_target(&mountpoint, &hit));
|
||||
});
|
||||
match glib::future_with_timeout(STAT_TIMEOUT, rx.recv()).await {
|
||||
Ok(Ok(target)) => target,
|
||||
Ok(Err(_)) => None,
|
||||
Err(_) => {
|
||||
tracing::warn!("the mount did not answer in time; opening through the daemon");
|
||||
None
|
||||
}
|
||||
}
|
||||
present_within(mounted_or_relative(&mountpoint, &hit)).await
|
||||
}
|
||||
|
||||
impl Ui {
|
||||
|
||||
@@ -361,8 +361,9 @@ fn open_manager(args: &[&str]) {
|
||||
.and_then(|p| p.parent().map(|d| d.join("pdfs-app")))
|
||||
.filter(|p| p.exists())
|
||||
.unwrap_or_else(|| PathBuf::from("pdfs-app"));
|
||||
if let Err(e) = Command::new(&exe).args(args).spawn() {
|
||||
tracing::error!("failed to launch {}: {e}", exe.display());
|
||||
match Command::new(&exe).args(args).spawn() {
|
||||
Ok(child) => pdfs_core::opener::reap(child),
|
||||
Err(e) => tracing::error!("failed to launch {}: {e}", exe.display()),
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -100,6 +100,7 @@ a live account since.
|
||||
|
||||
| Entry | Title |
|
||||
|---|---|
|
||||
| [B243](#b243--a-hung-mount-freezes-the-app-when-a-file-is-opened) | A hung mount freezes the app when a file is opened |
|
||||
| [B242](#b242--opening-a-large-photo-freezes-the-app) | Opening a large photo freezes the app |
|
||||
| [B241](#b241--every-closed-photo-viewer-stays-in-memory-with-its-photo) | Every closed photo viewer stays in memory with its photo |
|
||||
| [B240](#b240--pdfs-mount-can-be-core-dumped-with-the-keys-in-memory) | pdfs mount can be core-dumped with the keys in memory |
|
||||
@@ -345,6 +346,34 @@ a live account since.
|
||||
|
||||
Newest first.
|
||||
|
||||
### B243 — A hung mount freezes the app when a file is opened
|
||||
|
||||
**Status:** Fixed, not verified live
|
||||
**Found:** 2026-10-16, architecture audit (`FINDINGS.md` 2.5.3, and the unreaped child processes
|
||||
in section 3).
|
||||
**Where:** `present_within` in `crates/pdfs-gui/src/activation.rs`; `download_and_open` and
|
||||
`open_entry_with` in `crates/pdfs-gui/src/app/pages/browser.rs`; `open_path` in
|
||||
`crates/pdfs-gui/src/app/main.rs`; `reap` in `crates/pdfs-core/src/opener.rs`
|
||||
|
||||
**Repro:** Let the mount hang (a stalled link with a request stuck in the kernel), then
|
||||
double-click a file in the app's browser, or open the Proton Drive location. Separately, open a
|
||||
few files from the app and look for `<defunct>` children of `pdfs-app` in `ps`.
|
||||
|
||||
**Cause.** Opening a file asked the mount whether it had the file with a `stat` on the GTK main
|
||||
thread, and opening a location asked whether it was a folder the same way. A hung mount never
|
||||
answers, so the whole app hung with it. The search launcher already asked on a worker with a
|
||||
2 s limit. Separately, nothing waited for the applications the app, the tray and the player
|
||||
button started, so each one stayed a zombie until the app quit.
|
||||
|
||||
**Fix.** The browser's Open and Open With ask through `present_within`, which the launcher now
|
||||
shares: a thread of its own, given up on after 2 s, with the daemon as the fallback. Opening a
|
||||
location runs on a thread. Every child the GUI starts is waited for on a thread of its own.
|
||||
|
||||
**Test:** `opener::tests::a_reaped_child_does_not_stay_a_zombie`; the mount checks have no
|
||||
automated test, as the GUI tests run without a display.
|
||||
|
||||
---
|
||||
|
||||
### B242 — Opening a large photo freezes the app
|
||||
|
||||
**Status:** Fixed, not verified live
|
||||
|
||||
Reference in new issue
Block a user