Skip to content

fix: Explicitly drop SessionFlusher #326

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 26, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 16 additions & 6 deletions sentry-core/src/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ pub(crate) type TransportArc = Arc<RwLock<Option<Arc<dyn Transport>>>>;
pub struct Client {
options: ClientOptions,
transport: TransportArc,
session_flusher: SessionFlusher,
session_flusher: RwLock<Option<SessionFlusher>>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to somehow leave an implementation note that explains why this is an option? If I read this correctly it only is an option because it allows you to explicitly drop the SessionFlusher on close. So this rather different from the thing being actually optional as you'd expect from an option.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed. Although maybe it would be better to just make client a newtype around Option<ClientInner> or something.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that sounds even nicer, also more work though ;)

integrations: Vec<(TypeId, Arc<dyn Integration>)>,
sdk_info: ClientSdkInfo,
}
Expand All @@ -60,7 +60,10 @@ impl fmt::Debug for Client {
impl Clone for Client {
fn clone(&self) -> Client {
let transport = Arc::new(RwLock::new(self.transport.read().unwrap().clone()));
let session_flusher = SessionFlusher::new(transport.clone(), self.options.session_mode);
let session_flusher = RwLock::new(Some(SessionFlusher::new(
transport.clone(),
self.options.session_mode,
)));
Client {
options: self.options.clone(),
transport,
Expand Down Expand Up @@ -128,7 +131,10 @@ impl Client {
sdk_info.integrations.push(integration.name().to_string());
}

let session_flusher = SessionFlusher::new(transport.clone(), options.session_mode);
let session_flusher = RwLock::new(Some(SessionFlusher::new(
transport.clone(),
options.session_mode,
)));
Client {
options,
transport,
Expand Down Expand Up @@ -287,12 +293,16 @@ impl Client {
}

pub(crate) fn enqueue_session(&self, session_update: SessionUpdate<'static>) {
self.session_flusher.enqueue(session_update)
if let Some(ref flusher) = *self.session_flusher.read().unwrap() {
flusher.enqueue(session_update);
}
}

/// Drains all pending events without shutting down.
pub fn flush(&self, timeout: Option<Duration>) -> bool {
self.session_flusher.flush();
if let Some(ref flusher) = *self.session_flusher.read().unwrap() {
flusher.flush();
}
if let Some(ref transport) = *self.transport.read().unwrap() {
transport.flush(timeout.unwrap_or(self.options.shutdown_timeout))
} else {
Expand All @@ -308,7 +318,7 @@ impl Client {
/// If no timeout is provided the client will wait for as long a
/// `shutdown_timeout` in the client options.
pub fn close(&self, timeout: Option<Duration>) -> bool {
self.session_flusher.flush();
drop(self.session_flusher.write().unwrap().take());
let transport_opt = self.transport.write().unwrap().take();
if let Some(transport) = transport_opt {
sentry_debug!("client close; request transport to shut down");
Expand Down
2 changes: 1 addition & 1 deletion sentry-core/src/hub.rs
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ impl Hub {
f(&PROCESS_HUB.0)
} else {
// note on safety: this is safe because even though we change the Arc
// by temorary binding we guarantee that the original Arc stays alive.
// by temporary binding we guarantee that the original Arc stays alive.
// For more information see: run
THREAD_HUB.with(|stack| unsafe {
let ptr = stack.get();
Expand Down
2 changes: 1 addition & 1 deletion sentry/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ log_ = { package = "log", version = "0.4.8", optional = true, features = ["std"]
reqwest_ = { package = "reqwest", version = "0.11", optional = true, features = ["blocking", "json"], default-features = false }
curl_ = { package = "curl", version = "0.4.25", optional = true }
surf_ = { package = "surf", version = "2.0.0", optional = true }
httpdate = { version = "0.3.2", optional = true }
httpdate = { version = "1.0.0", optional = true }
serde_json = { version = "1.0.48", optional = true }
tokio = { version = "1.0", features = ["rt"] }

Expand Down
15 changes: 15 additions & 0 deletions sentry/tests/test_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,3 +56,18 @@ fn test_unwind_safe() {

assert_eq!(events.len(), 1);
}

#[test]
fn test_concurrent_init() {
let _guard = sentry::init(sentry::ClientOptions {
..Default::default()
});

std::thread::spawn(|| {
let _guard = sentry::init(sentry::ClientOptions {
..Default::default()
});
})
.join()
.unwrap();
}