Skip to content

Commit

Permalink
fix: await the tasks after aborting them
Browse files Browse the repository at this point in the history
  • Loading branch information
link2xt committed Jul 30, 2024
1 parent c163438 commit 490f41c
Show file tree
Hide file tree
Showing 3 changed files with 29 additions and 11 deletions.
15 changes: 10 additions & 5 deletions src/accounts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -167,12 +167,17 @@ impl Accounts {
.with_context(|| format!("no account with id {id}"))?;
ctx.stop_io().await;

// Explicitly close the database.
// Explicitly close the database
// to make sure the database file is closed
// and can be removed on Windows.
// If some spawned task tries to use the database afterwards,
// it will fail.
//
// Stopping I/O aborts the tasks that keep `Context` clones,
// but aborting the task does not immediately drops the future.
// To make sure the database file is closed
// and can be removed on Windows, we drop all the connections manually.
// Previously `stop_io()` aborted the tasks without awaiting them
// and this resulted in keeping `Context` clones inside
// `Future`s that were not dropped. This bug is fixed now,
// but explicitly closing the database ensures that file is freed
// even if not all `Context` references are dropped.
ctx.sql.close().await;
drop(ctx);

Expand Down
7 changes: 6 additions & 1 deletion src/contact.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1913,8 +1913,13 @@ impl RecentlySeenLoop {
.unwrap();
}

pub(crate) fn abort(self) {
pub(crate) async fn abort(self) {
self.handle.abort();

// Await aborted task to ensure the `Future` is dropped
// with all resources moved inside such as the `Context`
// reference to `InnerContext`.
self.handle.await.ok();
}
}

Expand Down
18 changes: 13 additions & 5 deletions src/scheduler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -110,13 +110,14 @@ impl SchedulerState {
// to allow for clean shutdown.
context.new_msgs_notify.notify_one();

if let Some(debug_logging) = context
let debug_logging = context
.debug_logging
.read()
.write()
.expect("RwLock is poisoned")
.as_ref()
{
.take();
if let Some(debug_logging) = debug_logging {
debug_logging.loop_handle.abort();
debug_logging.loop_handle.await.ok();
}
let prev_state = std::mem::replace(&mut *inner, new_state);
context.emit_event(EventType::ConnectivityChanged);
Expand Down Expand Up @@ -974,9 +975,16 @@ impl Scheduler {
.await
.log_err(context)
.ok();

// Abort tasks, then await them to ensure the `Future` is dropped.
// Just aborting the task may keep resources such as `Context` clone
// moved into it indefinitely, resulting in database not being
// closed etc.
self.ephemeral_handle.abort();
self.ephemeral_handle.await.ok();
self.location_handle.abort();
self.recently_seen_loop.abort();
self.location_handle.await.ok();
self.recently_seen_loop.abort().await;
}
}

Expand Down

0 comments on commit 490f41c

Please sign in to comment.