Skip to content
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

apply tower best practices to services when cloning inner services #2030

Merged
merged 6 commits into from
Nov 2, 2022
Merged
Show file tree
Hide file tree
Changes from 4 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
4 changes: 2 additions & 2 deletions NEXT_CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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))
garypen marked this conversation as resolved.
Show resolved Hide resolved

[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))

Expand Down
5 changes: 4 additions & 1 deletion apollo-router/src/services/execution_service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
4 changes: 3 additions & 1 deletion apollo-router/src/services/subgraph_service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,9 @@ impl tower::Service<crate::SubgraphRequest> 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 {
Expand Down
13 changes: 4 additions & 9 deletions apollo-router/src/services/supergraph_service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@ pub(crate) type Plugins = IndexMap<String, Box<dyn DynPlugin>>;
pub(crate) struct SupergraphService<ExecutionFactory> {
execution_service_factory: ExecutionFactory,
query_planner_service: CachingQueryPlanner<BridgeQueryPlanner>,
ready_query_planner_service: Option<CachingQueryPlanner<BridgeQueryPlanner>>,
schema: Arc<Schema>,
}

Expand All @@ -72,7 +71,6 @@ impl<ExecutionFactory> SupergraphService<ExecutionFactory> {
SupergraphService {
query_planner_service,
execution_service_factory,
ready_query_planner_service: None,
schema,
}
}
Expand All @@ -87,19 +85,16 @@ where
type Future = BoxFuture<'static, Result<Self::Response, Self::Error>>;

fn poll_ready(&mut self, cx: &mut std::task::Context<'_>) -> Poll<Result<(), Self::Error>> {
// 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
Copy link
Contributor

Choose a reason for hiding this comment

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

this is much cleaner 👍

.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();
Expand Down