From 2a52480f8ca154c0f3bcf4c3350a7ac8ad0cb407 Mon Sep 17 00:00:00 2001 From: Arpad Borsos Date: Mon, 24 Jan 2022 16:59:35 +0100 Subject: [PATCH 1/4] fix: Prevent Memory leaks from sentry-tower The TowerService::call seems to just create futures, and is not running in the context of a parent layers future. --- sentry-tower/src/http.rs | 93 ++++++++++++++++++++++------------------ 1 file changed, 51 insertions(+), 42 deletions(-) diff --git a/sentry-tower/src/http.rs b/sentry-tower/src/http.rs index d5cfd721..18a098cd 100644 --- a/sentry-tower/src/http.rs +++ b/sentry-tower/src/http.rs @@ -57,6 +57,10 @@ impl Layer for SentryHttpLayer { /// The Future returned from [`SentryHttpService`]. #[pin_project::pin_project] pub struct SentryHttpFuture { + on_first_poll: Option<( + sentry_core::protocol::Request, + Option, + )>, transaction: Option<( sentry_core::TransactionOrSpan, Option, @@ -73,6 +77,24 @@ where fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll { let slf = self.project(); + if let Some((sentry_req, trx_ctx)) = slf.on_first_poll.take() { + sentry_core::configure_scope(|scope| { + scope.add_event_processor(move |mut event| { + if event.request.is_none() { + event.request = Some(sentry_req.clone()); + } + Some(event) + }); + + if let Some(trx_ctx) = trx_ctx { + let transaction: sentry_core::TransactionOrSpan = + sentry_core::start_transaction(trx_ctx).into(); + let parent_span = scope.get_span(); + scope.set_span(Some(transaction.clone())); + *slf.transaction = Some((transaction, parent_span)); + } + }); + } match slf.future.poll(cx) { Poll::Ready(res) => { if let Some((transaction, parent_span)) = slf.transaction.take() { @@ -106,51 +128,38 @@ where } fn call(&mut self, request: Request) -> Self::Future { - let transaction = sentry_core::configure_scope(|scope| { - let sentry_req = sentry_core::protocol::Request { - method: Some(request.method().to_string()), - url: request.uri().to_string().parse().ok(), - headers: request - .headers() - .into_iter() - .map(|(header, value)| { - ( - header.to_string(), - value.to_str().unwrap_or_default().into(), - ) - }) - .collect(), - ..Default::default() - }; - scope.add_event_processor(move |mut event| { - if event.request.is_none() { - event.request = Some(sentry_req.clone()); - } - Some(event) + let sentry_req = sentry_core::protocol::Request { + method: Some(request.method().to_string()), + url: request.uri().to_string().parse().ok(), + headers: request + .headers() + .into_iter() + .map(|(header, value)| { + ( + header.to_string(), + value.to_str().unwrap_or_default().into(), + ) + }) + .collect(), + ..Default::default() + }; + let trx_ctx = if self.start_transaction { + let headers = request.headers().into_iter().flat_map(|(header, value)| { + value.to_str().ok().map(|value| (header.as_str(), value)) }); - - if self.start_transaction { - let headers = request.headers().into_iter().flat_map(|(header, value)| { - value.to_str().ok().map(|value| (header.as_str(), value)) - }); - let tx_name = format!("{} {}", request.method(), request.uri().path()); - let tx_ctx = sentry_core::TransactionContext::continue_from_headers( - &tx_name, - "http.server", - headers, - ); - let transaction: sentry_core::TransactionOrSpan = - sentry_core::start_transaction(tx_ctx).into(); - let parent_span = scope.get_span(); - scope.set_span(Some(transaction.clone())); - Some((transaction, parent_span)) - } else { - None - } - }); + let tx_name = format!("{} {}", request.method(), request.uri().path()); + Some(sentry_core::TransactionContext::continue_from_headers( + &tx_name, + "http.server", + headers, + )) + } else { + None + }; SentryHttpFuture { - transaction, + on_first_poll: Some((sentry_req, trx_ctx)), + transaction: None, future: self.service.call(request), } } From f48ecadf3f32bc64280d9c88da718769496fb0c7 Mon Sep 17 00:00:00 2001 From: Arpad Borsos Date: Mon, 24 Jan 2022 17:16:36 +0100 Subject: [PATCH 2/4] add example and more docs --- sentry-tower/src/lib.rs | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/sentry-tower/src/lib.rs b/sentry-tower/src/lib.rs index 3dba2023..b3ec8668 100644 --- a/sentry-tower/src/lib.rs +++ b/sentry-tower/src/lib.rs @@ -97,6 +97,24 @@ //! # Ok(()) //! # } //! ``` +//! +//! ## Usage with `tower-http` +//! +//! The `http` feature offers another layer which will attach request details +//! onto captured events, and optionally start a new performance monitoring +//! transaction based on the incoming HTTP headers. +//! +//! When combining both layers, take care of the ordering of both. For example +//! with [`tower::ServiceBuilder`], always define the `Hub` layer before the `Http` +//! one, like so: +//! +//! ```rust +//! # #[cfg(feature = "http")] { +//! let layer = tower::ServiceBuilder::new() +//! .layer(sentry_tower::NewSentryLayer::new_from_top()) +//! .layer(sentry_tower::SentryHttpLayer::with_transaction()); +//! # } +//! ``` #![doc(html_favicon_url = "https://sentry-brand.storage.googleapis.com/favicon.ico")] #![doc(html_logo_url = "https://sentry-brand.storage.googleapis.com/sentry-glyph-black.png")] From 696e2cda2b95b60e75ce4d502ec0be3e8cfc72e0 Mon Sep 17 00:00:00 2001 From: Arpad Borsos Date: Mon, 24 Jan 2022 17:26:27 +0100 Subject: [PATCH 3/4] correct doctest --- sentry-tower/src/lib.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/sentry-tower/src/lib.rs b/sentry-tower/src/lib.rs index b3ec8668..5d419cec 100644 --- a/sentry-tower/src/lib.rs +++ b/sentry-tower/src/lib.rs @@ -110,8 +110,9 @@ //! //! ```rust //! # #[cfg(feature = "http")] { +//! # type Request = http_::Request; //! let layer = tower::ServiceBuilder::new() -//! .layer(sentry_tower::NewSentryLayer::new_from_top()) +//! .layer(sentry_tower::NewSentryLayer::::new_from_top()) //! .layer(sentry_tower::SentryHttpLayer::with_transaction()); //! # } //! ``` From bd506cb4b5c8cecaaeff2fa8757b4454bd944e79 Mon Sep 17 00:00:00 2001 From: Arpad Borsos Date: Mon, 24 Jan 2022 18:29:27 +0100 Subject: [PATCH 4/4] run inner service call within the hub --- sentry-tower/src/lib.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/sentry-tower/src/lib.rs b/sentry-tower/src/lib.rs index 5d419cec..026cc2ef 100644 --- a/sentry-tower/src/lib.rs +++ b/sentry-tower/src/lib.rs @@ -253,8 +253,9 @@ where } fn call(&mut self, request: Request) -> Self::Future { - let hub = self.provider.hub(&request); - self.service.call(request).bind_hub(hub) + let hub = self.provider.hub(&request).into(); + let fut = Hub::run(hub.clone(), || self.service.call(request)); + fut.bind_hub(hub) } }