From ec4c42b93892c9812fc41e61dd6c068bef2b0afa Mon Sep 17 00:00:00 2001 From: Leonardo Yvens Date: Mon, 24 Apr 2023 19:42:30 -0300 Subject: [PATCH 1/3] ipfs: Remove concurrency limit, rely on rate limit The concurrency limit, because it uses a semaphore, had unfortunate interactions with the use of `call_all` in the polling monitor. `CallAll` can internally have requests which are holding the semaphore, so it must be polled regularly to check on those requests. If the task holding the `CallAll` hangs on some other future, that can lead to a deadlock. In our case, it can hang in the `response_sender.send((id, response)).await`, if the channel is full and the subgraph runner takes a long time to check it. This could be fixed, but the footgun would remain so it seemed best to rely only on the rate limit which can't deadlock. --- core/src/polling_monitor/ipfs_service.rs | 5 ++--- docs/environment-variables.md | 4 ++-- graph/src/env/mappings.rs | 2 +- 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/core/src/polling_monitor/ipfs_service.rs b/core/src/polling_monitor/ipfs_service.rs index 284d25063db..f9436b851ca 100644 --- a/core/src/polling_monitor/ipfs_service.rs +++ b/core/src/polling_monitor/ipfs_service.rs @@ -17,7 +17,7 @@ pub fn ipfs_service( client: IpfsClient, max_file_size: u64, timeout: Duration, - concurrency_and_rate_limit: u16, + rate_limit: u16, ) -> IpfsService { let ipfs = IpfsServiceInner { client, @@ -26,8 +26,7 @@ pub fn ipfs_service( }; let svc = ServiceBuilder::new() - .rate_limit(concurrency_and_rate_limit.into(), Duration::from_secs(1)) - .concurrency_limit(concurrency_and_rate_limit as usize) + .rate_limit(rate_limit.into(), Duration::from_secs(1)) .service_fn(move |req| ipfs.cheap_clone().call_inner(req)) .boxed(); diff --git a/docs/environment-variables.md b/docs/environment-variables.md index 635abc040c5..72880ab82be 100644 --- a/docs/environment-variables.md +++ b/docs/environment-variables.md @@ -78,8 +78,8 @@ those. may use (in bytes, defaults to 256MB). - `GRAPH_MAX_IPFS_CACHE_SIZE`: maximum number of files cached (defaults to 50). - `GRAPH_MAX_IPFS_CACHE_FILE_SIZE`: maximum size of each cached file (in bytes, defaults to 1MiB). -- `GRAPH_IPFS_REQUEST_LIMIT`: Limits both concurrent and per second requests to IPFS for file data - sources. Defaults to 100. +- `GRAPH_IPFS_REQUEST_LIMIT`: Limits both per second requests to IPFS for file data sources. + Defaults to 100. ## GraphQL diff --git a/graph/src/env/mappings.rs b/graph/src/env/mappings.rs index bb3ee2c1d30..639199d94c6 100644 --- a/graph/src/env/mappings.rs +++ b/graph/src/env/mappings.rs @@ -49,7 +49,7 @@ pub struct EnvVarsMapping { /// bytes). Defaults to 256 MiB. pub max_ipfs_file_bytes: usize, - /// Limits both concurrent and per second requests to IPFS for file data sources. + /// Limits per second requests to IPFS for file data sources. /// /// Set by the environment variable `GRAPH_IPFS_REQUEST_LIMIT`. Defaults to 100. pub ipfs_request_limit: u16, From 20bd8ccbde5e6fb6021237bda905e176a54305ea Mon Sep 17 00:00:00 2001 From: Leonardo Yvens Date: Mon, 24 Apr 2023 20:01:55 -0300 Subject: [PATCH 2/3] polling monitor: Log when not found --- core/src/polling_monitor/mod.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/core/src/polling_monitor/mod.rs b/core/src/polling_monitor/mod.rs index e50979d39f2..19f30f28cda 100644 --- a/core/src/polling_monitor/mod.rs +++ b/core/src/polling_monitor/mod.rs @@ -161,6 +161,8 @@ where // Object not found, push the id to the back of the queue. Ok((id, None)) => { + debug!(logger, "not found on polling"; "object_id" => id.to_string()); + metrics.not_found.inc(); queue.push_back(id); } From 344865eb74c4a156b19f6c3ff573862293bfa890 Mon Sep 17 00:00:00 2001 From: Leonardo Yvens Date: Wed, 26 Apr 2023 11:13:19 -0300 Subject: [PATCH 3/3] address review --- docs/environment-variables.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/environment-variables.md b/docs/environment-variables.md index 72880ab82be..83ee938f059 100644 --- a/docs/environment-variables.md +++ b/docs/environment-variables.md @@ -78,7 +78,7 @@ those. may use (in bytes, defaults to 256MB). - `GRAPH_MAX_IPFS_CACHE_SIZE`: maximum number of files cached (defaults to 50). - `GRAPH_MAX_IPFS_CACHE_FILE_SIZE`: maximum size of each cached file (in bytes, defaults to 1MiB). -- `GRAPH_IPFS_REQUEST_LIMIT`: Limits both per second requests to IPFS for file data sources. +- `GRAPH_IPFS_REQUEST_LIMIT`: Limits the number of requests per second to IPFS for file data sources. Defaults to 100. ## GraphQL