From 000836baedbc10760b8a422d4518571b1d5f99db Mon Sep 17 00:00:00 2001 From: Gary Pennington Date: Sat, 29 Oct 2022 10:18:30 +0100 Subject: [PATCH 1/3] apply tower best practices to services when cloning inner services When I was investigating connection resets I noted that we weren't following tower documentation with respect to cloning inner services: https://docs.rs/tower/latest/tower/trait.Service.html#be-careful-when-cloning-inner-services This PR addresses that. The change to the supergraph seems to be most invasive. I have run the tests manually many times and the change seems good, but probably worth closer examination. --- apollo-router/src/services/execution_service.rs | 5 ++++- apollo-router/src/services/subgraph_service.rs | 4 +++- apollo-router/src/services/supergraph_service.rs | 13 ++++--------- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/apollo-router/src/services/execution_service.rs b/apollo-router/src/services/execution_service.rs index 3460c27bc4..3b3fa67ede 100644 --- a/apollo-router/src/services/execution_service.rs +++ b/apollo-router/src/services/execution_service.rs @@ -56,7 +56,10 @@ where } fn call(&mut self, req: ExecutionRequest) -> Self::Future { - let this = self.clone(); + let clone = self.clone(); + + let this = std::mem::replace(self, clone); + let fut = async move { let context = req.context; let ctx = context.clone(); diff --git a/apollo-router/src/services/subgraph_service.rs b/apollo-router/src/services/subgraph_service.rs index 47403313d8..81fcc32651 100644 --- a/apollo-router/src/services/subgraph_service.rs +++ b/apollo-router/src/services/subgraph_service.rs @@ -108,7 +108,9 @@ impl tower::Service for SubgraphService { .. } = request; - let mut client = self.client.clone(); + let clone = self.client.clone(); + + let mut client = std::mem::replace(&mut self.client, clone); let service_name = (*self.service).to_owned(); Box::pin(async move { diff --git a/apollo-router/src/services/supergraph_service.rs b/apollo-router/src/services/supergraph_service.rs index 450943c5cf..3aa1dd0386 100644 --- a/apollo-router/src/services/supergraph_service.rs +++ b/apollo-router/src/services/supergraph_service.rs @@ -57,7 +57,6 @@ pub(crate) type Plugins = IndexMap>; pub(crate) struct SupergraphService { execution_service_factory: ExecutionFactory, query_planner_service: CachingQueryPlanner, - ready_query_planner_service: Option>, schema: Arc, } @@ -72,7 +71,6 @@ impl SupergraphService { SupergraphService { query_planner_service, execution_service_factory, - ready_query_planner_service: None, schema, } } @@ -87,19 +85,16 @@ where type Future = BoxFuture<'static, Result>; fn poll_ready(&mut self, cx: &mut std::task::Context<'_>) -> Poll> { - // We need to obtain references to two hot services for use in call. - // The reason for us to clone here is that the async block needs to own the hot services, - // and cloning will produce a cold service. Therefore cloning in `SupergraphService#call` is not - // a valid course of action. - self.ready_query_planner_service - .get_or_insert_with(|| self.query_planner_service.clone()) + self.query_planner_service .poll_ready(cx) .map_err(|err| err.into()) } fn call(&mut self, req: SupergraphRequest) -> Self::Future { // Consume our cloned services and allow ownership to be transferred to the async block. - let planning = self.ready_query_planner_service.take().unwrap(); + let clone = self.query_planner_service.clone(); + + let planning = std::mem::replace(&mut self.query_planner_service, clone); let execution = self.execution_service_factory.new_service(); let schema = self.schema.clone(); From 15145d870f7dae75444614354f8880654fd53740 Mon Sep 17 00:00:00 2001 From: Gary Pennington Date: Wed, 2 Nov 2022 09:47:42 +0000 Subject: [PATCH 2/3] fix the dhat changelog entry not worth raising a separate PR and this is still in flight... --- NEXT_CHANGELOG.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/NEXT_CHANGELOG.md b/NEXT_CHANGELOG.md index ab206de188..82a47ad5f6 100644 --- a/NEXT_CHANGELOG.md +++ b/NEXT_CHANGELOG.md @@ -28,11 +28,11 @@ By [@USERNAME](https://github.com/USERNAME) in https://github.com/apollographql/ ## ❗ BREAKING ❗ ## 🚀 Features -### Add support for dhat based heap profiling [PR #XXXX](https://github.com/apollographql/router/pull/XXXX)) +### Add support for dhat based heap profiling [PR #1829](https://github.com/apollographql/router/pull/1829)) [dhat-rs](https://github.com/nnethercote/dhat-rs) provides [DHAT](https://www.valgrind.org/docs/manual/dh-manual.html) style heap profiling. We have added two compile features, dhat-heap and dhat-ad-hoc, which leverage this ability. -By [@garypen](https://github.com/garypen) in https://github.com/apollographql/router/pull/XXXX +By [@garypen](https://github.com/garypen) in https://github.com/apollographql/router/pull/1829 ### Add `trace_id` in logs to identify all logs related to a specific request [Issue #1981](https://github.com/apollographql/router/issues/1981)) From 2817f79e3851a6a7c24db8e56e62390474c61d78 Mon Sep 17 00:00:00 2001 From: Gary Pennington Date: Wed, 2 Nov 2022 14:59:53 +0000 Subject: [PATCH 3/3] add a changelog --- NEXT_CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/NEXT_CHANGELOG.md b/NEXT_CHANGELOG.md index 82a47ad5f6..90a456024b 100644 --- a/NEXT_CHANGELOG.md +++ b/NEXT_CHANGELOG.md @@ -78,6 +78,12 @@ By [@fernando-apollo](https://github.com/fernando-apollo) in https://github.com/ ## 🛠 Maintenance +### Apply tower best practice to inner service cloning [PR #2030](https://github.com/apollographql/router/pull/2030)) + +Our service readiness checking can be improved by following tower project recommendations for cloning inner services. + +By [@garypen](https://github.com/garypen) in https://github.com/apollographql/router/pull/2030 + ### Split the configuration file management in multiple modules [Issue #1790](https://github.com/apollographql/router/issues/1790)) The file is becoming large and hard to modify.