Skip to content

Commit

Permalink
fix: Dont panic on browser drop (mattsse#139)
Browse files Browse the repository at this point in the history
  • Loading branch information
@Sytten
Sytten authored Feb 13, 2023
1 parent 021fb0d commit 84b9f4e
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 116 deletions.
68 changes: 15 additions & 53 deletions src/async_process.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,13 @@ pub struct Command {

impl Command {
pub fn new<S: AsRef<OsStr>>(program: S) -> Self {
let inner = process::Command::new(program);
let mut inner = process::Command::new(program);
// Since the kill and/or wait methods are async, we can't call
// explicitely in the Drop implementation. We MUST rely on the
// runtime implemetation which is already designed to deal with
// this case where the user didn't explicitely kill the child
// process before dropping the handle.
inner.kill_on_drop(true);
Self { inner }
}

Expand Down Expand Up @@ -78,40 +84,22 @@ impl Child {
}
}

/// Kill the child process, asynchronously if possible (otherwise by blocking)
///
/// - `async-std-runtime`: blocking call to `async_std::process::Child::kill`.
/// - `tokio-runtime`: async call to `tokio::process::Child::kill`
/// Kill the child process synchronously and asynchronously wait for the
/// child to exit
pub async fn kill(&mut self) -> std::io::Result<()> {
cfg_if::cfg_if! {
if #[cfg(feature = "async-std-runtime")] {
self.inner.kill()
self.inner.kill()?;
self.wait().await?;
Ok(())
} else if #[cfg(feature = "tokio-runtime")] {
// Tokio already waits internally
self.inner.kill().await
}
}
}

/// Kill the child process synchronously (blocking)
///
/// - `async-std-runtime`: blocking call to `async_std::process::Child::kill`.
/// - `tokio-runtime`: async call to `tokio::process::Child::kill`, resolved with `tokio::runtime::Handle::block_on`
pub fn kill_sync(&mut self) -> std::io::Result<()> {
cfg_if::cfg_if! {
if #[cfg(feature = "async-std-runtime")] {
self.inner.kill()
} else if #[cfg(feature = "tokio-runtime")] {
let fut = self.kill();
let handle = tokio::runtime::Handle::current();
handle.block_on(fut)
}
}
}

/// Asynchronously wait for the child process to exit (non-blocking)
///
/// - `async-std-runtime`: async call to `async_std::process::Child::status`.
/// - `tokio-runtime`: async call to `tokio::process::Child::wait`
/// Asynchronously wait for the child process to exit
pub async fn wait(&mut self) -> std::io::Result<ExitStatus> {
cfg_if::cfg_if! {
if #[cfg(feature = "async-std-runtime")] {
Expand All @@ -122,27 +110,7 @@ impl Child {
}
}

/// Synchronously wait for the child process to exit (blocking)
///
/// - `async-std-runtime`: async call to `async_std::process::Child::status`, resolved with `async_std::task::block_on`
/// - `tokio-runtime`: async call to `tokio::process::Child::wait`, resolved with `tokio::runtime::Handle::block_on`
pub fn wait_sync(&mut self) -> std::io::Result<ExitStatus> {
cfg_if::cfg_if! {
if #[cfg(feature = "async-std-runtime")] {
let fut = self.wait();
async_std::task::block_on(fut)
} else if #[cfg(feature = "tokio-runtime")] {
let fut = self.wait();
let handle = tokio::runtime::Handle::current();
handle.block_on(fut)
}
}
}

/// If the child process has exited, get its status (non-blocking)
///
/// - `async-std-runtime`: call to `async_std::process::Child::try_status`
/// - `tokio-runtime`: call to `tokio::process::Child::try_wait`
/// If the child process has exited, get its status
pub fn try_wait(&mut self) -> std::io::Result<Option<ExitStatus>> {
cfg_if::cfg_if! {
if #[cfg(feature = "async-std-runtime")] {
Expand All @@ -156,17 +124,11 @@ impl Child {
/// Return a mutable reference to the inner process
///
/// `stderr` may not be available.
///
/// - `async-std-runtime`: return `&mut async_std::process::Child`
/// - `tokio-runtime`: return `&mut tokio::process::Child`
pub fn as_mut_inner(&mut self) -> &mut process::Child {
&mut self.inner
}

/// Return the inner process
///
/// - `async-std-runtime`: return `async_std::process::Child`
/// - `tokio-runtime`: return `tokio::process::Child`
pub fn into_inner(self) -> process::Child {
let mut inner = self.inner;
inner.stderr = self.stderr.map(ChildStderr::into_inner);
Expand Down
74 changes: 11 additions & 63 deletions src/browser.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -177,9 +177,6 @@ impl Browser {
/// [`Browser::close`]. You can call this explicitly to collect the process and avoid
/// "zombie" processes.
///
/// This method is not blocking. Its behavior depends on the selected runtime, see
/// [`async_process::Child::wait`] for details.
///
/// This call has no effect if this [`Browser`] did not spawn any chromium instance (e.g.
/// connected to an existing browser through [`Browser::connect`])
pub async fn wait(&mut self) -> io::Result<Option<ExitStatus>> {
Expand All @@ -190,35 +187,12 @@ impl Browser {
}
}

/// Synchronously wait for the spawned chromium instance to exit completely.
///
/// The instance is spawned by [`Browser::launch`]. `wait_sync` is usually called after
/// [`Browser::close`]. You can call this explicitly to collect the process and avoid
/// "zombie" processes. `wait_sync` is called automatically as part of [`Browser::drop`]
/// if needed.
///
/// This method is blocking. Its behavior depends on the selected runtime, see
/// [`async_process::Child::wait_sync`] for details.
///
/// This call has no effect if this [`Browser`] did not spawn any chromium instance (e.g.
/// connected to an existing browser through [`Browser::connect`])
pub fn wait_sync(&mut self) -> io::Result<Option<ExitStatus>> {
if let Some(child) = self.child.as_mut() {
Ok(Some(child.wait_sync()?))
} else {
Ok(None)
}
}

/// If the spawned chromium instance has completely exited, wait for it.
///
/// The instance is spawned by [`Browser::launch`]. `try_wait` is usually called after
/// [`Browser::close`]. You can call this explicitly to collect the process and avoid
/// "zombie" processes.
///
/// This method is not blocking. Its behavior depends on the selected runtime, see
/// [`async_process::Child::try_wait`] for details.
///
/// This call has no effect if this [`Browser`] did not spawn any chromium instance (e.g.
/// connected to an existing browser through [`Browser::connect`])
pub fn try_wait(&mut self) -> io::Result<Option<ExitStatus>> {
Expand All @@ -245,15 +219,12 @@ impl Browser {

/// Forcibly kill the spawned chromium instance
///
/// The instance is spawned by [`Browser::launch`]. `kill` is usually followed by
/// [`Browser::wait`] or [`Browser::try_wait`] to avoid "zombie" processes.
/// The instance is spawned by [`Browser::launch`]. `kill` will automatically wait for the child
/// process to exit to avoid "zombie" processes.
///
/// This method is provided to help if the browser does not close by itself. You should prefer
/// to use [`Browser::close`].
///
/// **This method may or may not be blocking.** The behavior depends on the selected runtime,
/// see [`async_process::Child::kill`] for details.
///
/// This call has no effect if this [`Browser`] did not spawn any chromium instance (e.g.
/// connected to an existing browser through [`Browser::connect`])
pub async fn kill(&mut self) -> Option<io::Result<()>> {
Expand All @@ -263,24 +234,6 @@ impl Browser {
}
}

/// Forcibly kill the spawned chromium instance
///
/// The instance is spawned by [`Browser::launch`]. `kill` is usually followed by
/// [`Browser::wait_sync`] or [`Browser::try_wait`] to avoid "zombie" processes. It is called
/// automatically as part of [`Browser::drop`] if needed.
///
/// This method is provided to help if the browser does not close by itself. You should prefer
/// to use [`Browser::close`].
///
/// This method is blocking. The behavior depends on the selected runtime,
/// see [`async_process::Child::kill`] for details.
///
/// This call has no effect if this [`Browser`] did not spawn any chromium instance (e.g.
/// connected to an existing browser through [`Browser::connect`])
pub fn kill_sync(&mut self) -> Option<io::Result<()>> {
self.child.as_mut().map(|child| child.kill_sync())
}

/// If not launched as incognito this creates a new incognito browser
/// context. After that this browser exists within the incognito session.
/// New pages created while being in incognito mode will also run in the
Expand Down Expand Up @@ -422,21 +375,16 @@ impl Drop for Browser {
fn drop(&mut self) {
if let Some(child) = self.child.as_mut() {
if let Ok(Some(_)) = child.try_wait() {
// already exited, do nothing. Usually occurs after using the method close.
// If there is a method to detect whether the child handle is still available, it should be used instead of try_wait.
// Already exited, do nothing. Usually occurs after using the method close or kill.
} else {
child.kill_sync().expect("!kill");
// important to wait other wise kill will leave zombie process in system
// this is blocking call and we dont have any choice in the drop function
// one way is to do something like
// tokio::task::spawn_blocking(move || {
// drop(browser);
// });
// otherwise the end developer will have to make sure
// child process is killed either by manually calling kill() and wait()
// or call kill() and then repeatedly calling try_wait() until it return true
// if developer wants true asynchronous version
child.wait_sync().expect("!wait");
// We set the `kill_on_drop` property for the child process, so no need to explicitely
// kill it here. It can't really be done anyway since the method is async.
//
// On Unix, the process will be reaped in the background by the runtime automatically
// so it won't leave any resources locked. It is, however, a better practice for the user to
// do it himself since the runtime doesn't provide garantees as to when the reap occurs, so we
// warn him here.
tracing::warn!("Browser was not closed manually, it will be killed automatically in the background");
}
}
}
Expand Down

0 comments on commit 84b9f4e

Please sign in to comment.