From 1586eab06284ce668779c87f00a1fb5fa9763be0 Mon Sep 17 00:00:00 2001 From: Vladislav Ivanov Date: Fri, 29 Sep 2023 23:08:58 +0200 Subject: [PATCH] Fix check-then-act race condition The semaphore is aquired after the conditions for fetching are already evaluated. It means that multiple threads/runners can decide to fetch simultaneously and then wait on the same semaphore, even though only one fetch is needed. Therefore, fetch would happen more often than what is specified in fetch times, and requests can be slow under load. commit-id:a4cd58e6 --- josh-proxy/src/bin/josh-proxy.rs | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/josh-proxy/src/bin/josh-proxy.rs b/josh-proxy/src/bin/josh-proxy.rs index 4e387b45..6fac078c 100644 --- a/josh-proxy/src/bin/josh-proxy.rs +++ b/josh-proxy/src/bin/josh-proxy.rs @@ -129,6 +129,15 @@ async fn fetch_upstream( let refs_to_fetch: Vec<_> = refs_to_fetch.iter().map(|x| x.to_string()).collect(); + let us = upstream_repo.clone(); + let semaphore = service + .fetch_permits + .lock()? + .entry(us.clone()) + .or_insert(Arc::new(tokio::sync::Semaphore::new(1))) + .clone(); + let permit = semaphore.acquire().await; + let fetch_timer_ok = { if let Some(last) = service.fetch_timers.read()?.get(&key) { let since = std::time::Instant::now().duration_since(*last); @@ -183,15 +192,7 @@ async fn fetch_upstream( let br_path = service.repo_path.join("mirror"); let span = tracing::span!(tracing::Level::TRACE, "fetch worker"); - let us = upstream_repo.clone(); let ru = remote_url.clone(); - let semaphore = service - .fetch_permits - .lock()? - .entry(us.clone()) - .or_insert(Arc::new(tokio::sync::Semaphore::new(1))) - .clone(); - let permit = semaphore.acquire().await; let task_remote_auth = remote_auth.clone(); let fetch_result = tokio::task::spawn_blocking(move || { let _span_guard = span.enter();