Skip to content

Commit

Permalink
Avoid a panic when trying to persist ping-lifetime data after shutdown
Browse files Browse the repository at this point in the history
This requires threading the error through our internal codebase.
Other internals blocking on the queue will still panic if that's not
possible. The only other functions that block on the dispatcher should
be test functions though.
  • Loading branch information
badboy committed Sep 21, 2021
1 parent 0a5b5e1 commit 4888805
Show file tree
Hide file tree
Showing 4 changed files with 57 additions and 10 deletions.
4 changes: 2 additions & 2 deletions glean-core/rlb/src/dispatcher/global.rs
Expand Up @@ -47,8 +47,8 @@ pub fn launch(task: impl FnOnce() + Send + 'static) {
}

/// Block until all tasks prior to this call are processed.
pub fn block_on_queue() {
guard().block_on_queue();
pub fn block_on_queue() -> Result<(), DispatchError> {
guard().block_on_queue()
}

/// Starts processing queued tasks in the global dispatch queue.
Expand Down
11 changes: 5 additions & 6 deletions glean-core/rlb/src/dispatcher/mod.rs
Expand Up @@ -156,7 +156,7 @@ impl DispatchGuard {
}
}

fn block_on_queue(&self) {
fn block_on_queue(&self) -> Result<(), DispatchError> {
let (tx, rx) = crossbeam_channel::bounded(0);

// We explicitly don't use `self.launch` here.
Expand All @@ -169,11 +169,10 @@ impl DispatchGuard {
.expect("(worker) Can't send message on single-use channel");
}));
self.sender
.send(task)
.expect("Failed to launch the blocking task");
.send(task)?;

rx.recv()
.expect("Failed to receive message on single-use channel");
rx.recv()?;
Ok(())
}

fn kill(&mut self) -> Result<(), DispatchError> {
Expand Down Expand Up @@ -326,7 +325,7 @@ impl Dispatcher {
self.guard.clone()
}

fn block_on_queue(&self) {
fn block_on_queue(&self) -> Result<(), DispatchError> {
self.guard().block_on_queue()
}

Expand Down
8 changes: 6 additions & 2 deletions glean-core/rlb/src/lib.rs
Expand Up @@ -424,7 +424,7 @@ fn block_on_dispatcher() {
was_initialize_called(),
"initialize was never called. Can't block on the dispatcher queue."
);
dispatcher::block_on_queue()
dispatcher::block_on_queue().unwrap();
}

/// Checks if [`initialize`] was ever called.
Expand Down Expand Up @@ -800,7 +800,11 @@ pub fn persist_ping_lifetime_data() -> Result<()> {
});
Ok(())
} else {
block_on_dispatcher();
// Calling the dispatcher directly to not panic on errors.
// Blocking on the queue will fail when the queue is already shutdown,
// which is equivalent to Glean not being initialized
// (In production Glean can't be re-initialized).
dispatcher::block_on_queue().map_err(|_| Error::not_initialized())?;
with_glean(|glean| glean.persist_ping_lifetime_data())
}
}
Expand Down
44 changes: 44 additions & 0 deletions glean-core/rlb/tests/persist_ping_lifetime_nopanic.rs
@@ -0,0 +1,44 @@
// This Source Code Form is subject to the terms of the Mozilla Public
// License, v. 2.0. If a copy of the MPL was not distributed with this
// file, You can obtain one at https://mozilla.org/MPL/2.0/.

//! This integration test should model how the RLB is used when embedded in another Rust application
//! (e.g. FOG/Firefox Desktop).
//!
//! We write a single test scenario per file to avoid any state keeping across runs
//! (different files run as different processes).

mod common;

use glean::Configuration;
use std::path::PathBuf;

fn cfg_new(tmpname: PathBuf) -> Configuration {
Configuration {
data_path: tmpname,
application_id: "firefox-desktop".into(),
upload_enabled: true,
max_events: None,
delay_ping_lifetime_io: true,
channel: Some("testing".into()),
server_endpoint: Some("invalid-test-host".into()),
uploader: None,
use_core_mps: false,
}
}

/// Test scenario: `persist_ping_lifetime_data` called after shutdown.
#[test]
fn delayed_ping_data() {
common::enable_test_logging();

// Create a custom configuration to delay ping-lifetime io
let dir = tempfile::tempdir().unwrap();
let tmpname = dir.path().to_path_buf();

common::initialize(cfg_new(tmpname));
assert!(glean::persist_ping_lifetime_data().is_ok());

glean::shutdown();
assert!(glean::persist_ping_lifetime_data().is_err());
}

0 comments on commit 4888805

Please sign in to comment.