Wait for the session to end when dropping BackgroundSession

CHANGELOG.md

@@ -1,6 +1,15 @@
 # FUSE for Rust - Changelog
 
 ## Unreleased
+* Dropping a `BackgroundSession` now unmounts the filesystem and waits for the session to
+  end, guaranteeing that `Filesystem::destroy` has run when drop returns (#239, #411). This
+  restores the pre-0.16 blocking drop behavior. Drop does not wait when the session cannot
+  end: sessions created via `Session::from_fd` are left detached (use `join()` after ending
+  them), and so are sessions whose unmount failed or whose connection is still alive a few
+  seconds after the unmount (e.g. a lazily unmounted filesystem still in use).
+  The `guard` field of `BackgroundSession` is now private - use `join()`/`umount_and_join()`
+* The pure-rust and `libfuse2` mount backends now report `fusermount` unmount failures
+  instead of silently ignoring them
 * Treat `ECONNABORTED` from the FUSE device as a clean session end (#212): with
   `FUSE_ABORT_ERROR` negotiated, aborting the connection made `Session::run()` and
   `BackgroundSession::umount_and_join()` return an error instead of ending normally

src/channel.rs

@@ -47,6 +47,11 @@ impl Channel {
         }
     }
 
+    /// Returns the FUSE device of this channel.
+    pub(crate) fn device(&self) -> Arc<DevFuse> {
+        self.0.clone()
+    }
+
     /// Returns a sender object for this channel. The sender object can be
     /// used to send to the channel. Multiple sender objects can be used
     /// and they can safely be sent to other threads.

src/mnt/fuse2.rs

@@ -51,8 +51,8 @@ impl MountImpl {
         // directly, which is what osxfuse does anyway, since we already converted
         // to the real path when we first mounted.
         if let Err(err) = crate::mnt::libc_umount(&self.mountpoint) {
-            // Linux always returns EPERM for non-root users.  We have to let the
-            // library go through the setuid-root "fusermount -u" to unmount.
+            // Linux always returns EPERM for non-root users.  We have to go
+            // through the setuid-root "fusermount -u" to unmount.
             if err == nix::errno::Errno::EPERM {
                 #[cfg(not(any(
                     target_os = "macos",
@@ -61,9 +61,12 @@ impl MountImpl {
                     target_os = "openbsd",
                     target_os = "netbsd"
                 )))]
-                unsafe {
-                    fuse_unmount_compat22(self.mountpoint.as_ptr());
-                    return Ok(());
+                {
+                    // Not libfuse's fuse_unmount_compat22: that would swallow
+                    // fusermount failures (leaving a still-running session that
+                    // callers then believe has been unmounted) and stat the
+                    // mountpoint through the filesystem via realpath
+                    return crate::mnt::fusermount_unmount("fusermount", &self.mountpoint);
                 }
             }
             return Err(err.into());

src/mnt/fuse2_sys.rs

@@ -23,12 +23,4 @@ unsafe extern "C" {
     // Therefore, the minimum version requirement for *_compat25 functions is libfuse-2.6.0.
 
     pub(crate) fn fuse_mount_compat25(mountpoint: *const c_char, args: *const fuse_args) -> c_int;
-    #[cfg(not(any(
-        target_os = "macos",
-        target_os = "freebsd",
-        target_os = "dragonfly",
-        target_os = "openbsd",
-        target_os = "netbsd"
-    )))]
-    pub(crate) fn fuse_unmount_compat22(mountpoint: *const c_char);
 }

src/mnt/fuse_pure.rs

@@ -84,7 +84,7 @@ impl MountImpl {
             if err == nix::errno::Errno::EPERM {
                 // Linux always returns EPERM for non-root users.  We have to let the
                 // library go through the setuid-root "fusermount -u" to unmount.
-                fuse_unmount_pure(&self.mountpoint);
+                fuse_unmount_pure(&self.mountpoint)?;
                 return Ok(());
             } else {
                 return Err(err.into());
@@ -120,33 +120,20 @@ fn fuse_mount_pure(
     fuse_mount_fusermount(mountpoint, options, acl)
 }
 
-fn fuse_unmount_pure(mountpoint: &CStr) {
+fn fuse_unmount_pure(mountpoint: &CStr) -> io::Result<()> {
     #[cfg(target_os = "linux")]
     {
         if nix::mount::umount2(mountpoint, nix::mount::MntFlags::MNT_DETACH).is_ok() {
-            return;
+            return Ok(());
         }
     }
     #[cfg(target_os = "macos")]
     {
         if nix::mount::unmount(mountpoint, nix::mount::MntFlags::MNT_FORCE).is_ok() {
-            return;
+            return Ok(());
         }
     }
-
-    let mut builder = Command::new(detect_fusermount_bin());
-    builder.stdout(Stdio::piped()).stderr(Stdio::piped());
-    builder
-        .arg("-u")
-        .arg("-q")
-        .arg("-z")
-        .arg("--")
-        .arg(OsStr::new(&mountpoint.to_string_lossy().into_owned()));
-
-    if let Ok(output) = builder.output() {
-        debug!("fusermount: {}", String::from_utf8_lossy(&output.stdout));
-        debug!("fusermount: {}", String::from_utf8_lossy(&output.stderr));
-    }
+    crate::mnt::fusermount_unmount(&detect_fusermount_bin(), mountpoint)
 }
 
 fn detect_fusermount_bin() -> String {

src/mnt/mod.rs

@@ -212,6 +212,75 @@ fn libc_umount(mnt: &CStr) -> nix::Result<()> {
     }
 }
 
+/// Waits (bounded) for the FUSE kernel connection to end after an unmount was
+/// requested. Returns false if the connection is still alive when the timeout
+/// expires, e.g. because a lazily unmounted filesystem is still in use, or an
+/// unmount helper failed in a way that cannot be observed directly.
+#[cfg(not(target_os = "macos"))]
+pub(crate) fn connection_ended(fuse_device: &DevFuse, timeout: std::time::Duration) -> bool {
+    let deadline = std::time::Instant::now() + timeout;
+    while is_mounted(fuse_device) {
+        if std::time::Instant::now() >= deadline {
+            return false;
+        }
+        std::thread::sleep(std::time::Duration::from_millis(10));
+    }
+    true
+}
+
+/// The FUSE device poll semantics are untested on macOS, so assume the unmount
+/// took effect there.
+#[cfg(target_os = "macos")]
+pub(crate) fn connection_ended(_fuse_device: &DevFuse, _timeout: std::time::Duration) -> bool {
+    true
+}
+
+/// Lazily unmount via the given fusermount binary. Helper failures are reported,
+/// so that callers do not mistake a still-mounted filesystem for an unmounted
+/// one - BackgroundSession would otherwise wait forever for the session to end.
+#[cfg(any(
+    all(test, target_os = "linux"),
+    fuser_mount_impl = "pure-rust",
+    all(
+        fuser_mount_impl = "libfuse2",
+        not(any(
+            target_os = "macos",
+            target_os = "freebsd",
+            target_os = "dragonfly",
+            target_os = "openbsd",
+            target_os = "netbsd"
+        ))
+    )
+))]
+pub(crate) fn fusermount_unmount(fusermount_bin: &str, mountpoint: &CStr) -> io::Result<()> {
+    use std::os::unix::ffi::OsStrExt;
+
+    use log::debug;
+
+    let mut builder = std::process::Command::new(fusermount_bin);
+    builder
+        .stdout(std::process::Stdio::piped())
+        .stderr(std::process::Stdio::piped());
+    builder
+        .arg("-u")
+        .arg("-q")
+        .arg("-z")
+        .arg("--")
+        .arg(std::ffi::OsStr::from_bytes(mountpoint.to_bytes()));
+
+    let output = builder.output()?;
+    debug!("fusermount: {}", String::from_utf8_lossy(&output.stdout));
+    debug!("fusermount: {}", String::from_utf8_lossy(&output.stderr));
+    if !output.status.success() {
+        return Err(io::Error::other(format!(
+            "fusermount failed to unmount ({}): {}",
+            output.status,
+            String::from_utf8_lossy(&output.stderr).trim()
+        )));
+    }
+    Ok(())
+}
+
 /// Warning: This will return true if the filesystem has been detached (lazy unmounted), but not
 /// yet destroyed by the kernel.
 #[cfg(not(target_os = "macos"))]
@@ -276,6 +345,23 @@ mod test {
         );
     }
 
+    /// A fusermount helper that cannot run or exits nonzero must report an error:
+    /// the filesystem is then still mounted, and treating that as success would
+    /// make teardown wait forever for a session that never ends.
+    #[test]
+    #[cfg(target_os = "linux")]
+    fn fusermount_unmount_reports_failure() {
+        use std::ffi::CString;
+
+        let mountpoint = CString::new("/nonexistent").unwrap();
+        // Helper exits nonzero (/bin/false ignores its arguments)
+        assert!(fusermount_unmount("/bin/false", &mountpoint).is_err());
+        // Helper cannot be executed at all
+        assert!(fusermount_unmount("/nonexistent/fusermount", &mountpoint).is_err());
+        // Helper reports success
+        assert!(fusermount_unmount("/bin/true", &mountpoint).is_ok());
+    }
+
     #[cfg(not(target_os = "macos"))]
     fn cmd_mount() -> String {
         std::str::from_utf8(

src/session.rs

@@ -227,17 +227,21 @@ impl<FS: Filesystem> Session<FS> {
     }
 
     /// Run the session loop in a background thread. If the returned handle is dropped,
-    /// the filesystem is unmounted and the given session ends.
+    /// the filesystem is unmounted and the session is waited for, so that
+    /// `Filesystem::destroy` has run when drop returns - except in the cases documented
+    /// on [`BackgroundSession`], where the session thread is detached instead.
     pub fn spawn(self) -> io::Result<BackgroundSession> {
         let sender = self.ch.sender();
+        let fuse_device = self.ch.device();
         // Take the fuse_session, so that we can unmount it
         let mount = std::mem::take(&mut *self.mount.mount.lock());
         let guard = thread::Builder::new()
             .name("fuser-bg".to_string())
             .spawn(move || self.run())?;
         Ok(BackgroundSession {
-            guard,
+            guard: Some(guard),
             sender,
+            fuse_device,
             mount,
         })
     }
@@ -565,33 +569,69 @@ impl<FS: Filesystem> SessionEventLoop<FS> {
 }
 
 /// The background session data structure
+///
+/// Dropping this unmounts the filesystem and blocks until the session has ended,
+/// which guarantees that `Filesystem::destroy` has run when drop returns. Because
+/// of that, it must not be dropped from within a filesystem callback, which runs
+/// on the session thread being waited for. For a session created via
+/// `Session::from_fd` there is no mount to remove, so dropping cannot end the
+/// session and leaves its thread detached; end the session externally and use
+/// `join` to wait for it instead. The thread is likewise left detached, with a
+/// warning, rather than waiting for a session that may never end when the
+/// unmount fails, or when the kernel connection is still alive a few seconds
+/// after a successful unmount request (e.g. a lazily unmounted filesystem that
+/// is still in use, or an unmount helper that failed without reporting it).
 #[derive(Debug)]
 pub struct BackgroundSession {
-    /// Thread guard of the background session
-    pub guard: JoinHandle<io::Result<()>>,
+    /// Thread guard of the background session. None once joined.
+    guard: Option<JoinHandle<io::Result<()>>>,
     /// Object for creating Notifiers for client use
     sender: ChannelSender,
+    /// The FUSE device fd of the session's kernel connection
+    fuse_device: Arc<DevFuse>,
     /// Ensures the filesystem is unmounted when the session ends
     mount: Option<Mount>,
 }
 
+/// How long teardown waits for the kernel connection to end after a successful
+/// unmount request, before giving up on joining the session thread. The
+/// connection can end asynchronously (auto_unmount, kernel teardown), but a few
+/// seconds only pass without it ending when the session cannot be waited for,
+/// e.g. a lazily unmounted filesystem still in use
+const UNMOUNT_WAIT: std::time::Duration = std::time::Duration::from_secs(5);
+
 impl BackgroundSession {
-    /// Unmount the filesystem and join the background thread.
+    /// Unmount the filesystem and join the background thread, returning the
+    /// session result. `Filesystem::destroy` has run when this returns.
     pub fn umount_and_join(mut self) -> io::Result<()> {
         if let Some(mount) = self.mount.take() {
-            mount.umount()?;
+            if let Err(err) = mount.umount() {
+                // The filesystem is still mounted and the session still running,
+                // so joining would block indefinitely; leave the thread detached,
+                // as before
+                self.guard.take();
+                return Err(err);
+            }
         }
-        self.join()
+        self.join_impl()
     }
 
     /// Returns an object that can be used to send notifications to the kernel
     pub fn notifier(&self) -> Notifier {
         Notifier::new(self.sender.clone())
     }
 
-    /// Join the filesystem thread.
-    pub fn join(self) -> io::Result<()> {
-        self.guard
+    /// Join the filesystem thread without unmounting first: blocks until
+    /// something else ends the session, e.g. an external unmount.
+    pub fn join(mut self) -> io::Result<()> {
+        self.join_impl()
+    }
+
+    fn join_impl(&mut self) -> io::Result<()> {
+        let Some(guard) = self.guard.take() else {
+            return Ok(());
+        };
+        guard
             .join()
             .map_err(|_panic: Box<dyn std::any::Any + Send>| {
                 io::Error::new(
@@ -602,6 +642,36 @@ impl BackgroundSession {
     }
 }
 
+impl Drop for BackgroundSession {
+    fn drop(&mut self) {
+        // Unmount and wait for the session to end, so that Filesystem::destroy
+        // has run by the time drop returns (issues #239 and #411). Without the
+        // join, the process could exit before the detached session thread got
+        // around to calling destroy
+        let Some(mount) = self.mount.take() else {
+            // No mount to remove (Session::from_fd): nothing here can end the
+            // session, so joining could block forever; leave the thread detached
+            return;
+        };
+        if let Err(err) = mount.umount() {
+            // Still mounted, so the session is still running and joining would
+            // block indefinitely
+            warn!("Failed to unmount filesystem during drop: {err}");
+            return;
+        }
+        // The connection can end asynchronously (e.g. auto_unmount), and some
+        // unmount helpers cannot report failures. Wait boundedly, and if the
+        // session lives on, detach rather than block indefinitely
+        if !crate::mnt::connection_ended(&self.fuse_device, UNMOUNT_WAIT) {
+            warn!("FUSE connection still alive after unmount; detaching session thread");
+            return;
+        }
+        if let Err(err) = self.join_impl() {
+            warn!("Session ended with an error during drop: {err}");
+        }
+    }
+}
+
 // The abort test uses fusectl, which only exists on Linux; on other targets the
 // whole module would be dead code
 #[cfg(all(test, target_os = "linux"))]
@@ -675,6 +745,40 @@ mod test {
         ManuallyDrop::into_inner(tmp);
     }
 
+    /// Dropping a BackgroundSession must unmount the filesystem and wait for the
+    /// session to end, so that Filesystem::destroy has run when drop returns.
+    #[test]
+    fn drop_waits_for_destroy() {
+        struct DestroyFs {
+            destroyed: Arc<std::sync::atomic::AtomicBool>,
+        }
+        impl Filesystem for DestroyFs {
+            fn destroy(&mut self) {
+                // Give drop a chance to return early: without the join, drop
+                // consistently wins this race and the assertion below fails
+                std::thread::sleep(std::time::Duration::from_millis(200));
+                self.destroyed
+                    .store(true, std::sync::atomic::Ordering::SeqCst);
+            }
+        }
+
+        let tmp = ManuallyDrop::new(tempfile::tempdir().unwrap());
+        let destroyed = Arc::new(std::sync::atomic::AtomicBool::new(false));
+        let fs = DestroyFs {
+            destroyed: destroyed.clone(),
+        };
+        let bg = Session::new(fs, tmp.path(), &Config::default())
+            .unwrap()
+            .spawn()
+            .unwrap();
+        drop(bg);
+        assert!(
+            destroyed.load(std::sync::atomic::Ordering::SeqCst),
+            "destroy() must have been called by the time drop returns"
+        );
+        ManuallyDrop::into_inner(tmp);
+    }
+
     /// The fusectl abort file for the FUSE mount at `mountpoint`: the connection
     /// directory is named after the mount's anonymous device number.
     fn fusectl_abort_path(mountpoint: &Path) -> Option<std::path::PathBuf> {