refactor(remote-filesystem): return headers from copy/get_contents

Wrap RemoteFilesystem in Rc<RefCell<>> inside HttpDownloader, in prep
for the upcoming &self conversion of add()/get(). copy()/get_contents()
now bundle the response headers into their return value instead of
requiring a follow-up get_last_headers() call, since two separate calls
through a shared RefCell could otherwise race: nothing would guarantee
the reader observes the headers from its own request rather than one
clobbered by a concurrently borrowed call. get_last_headers() itself is
left in place, mirroring RemoteFilesystem::getLastHeaders() in PHP.

2 files changed, 29 insertions, 18 deletions

diff --git a/crates/shirabe/src/util/http_downloader.rs b/crates/shirabe/src/util/http_downloader.rs
index eeb4b6f0..9e2161c7 100644
--- a/crates/shirabe/src/util/http_downloader.rs
+++ b/crates/shirabe/src/util/http_downloader.rs
@@ -42,7 +42,7 @@ pub struct HttpDownloader {
     /// @var ?CurlDownloader
     curl: Option<CurlDownloader>,
     /// @var ?RemoteFilesystem
-    rfs: Option<RemoteFilesystem>,
+    rfs: Option<std::rc::Rc<std::cell::RefCell<RemoteFilesystem>>>,
     /// @var int
     id_gen: i64,
     /// @var bool
@@ -174,13 +174,15 @@ impl HttpDownloader {
             None
         };
 
-        let rfs = Some(RemoteFilesystem::new(
-            io.clone(),
-            config.clone(),
-            options.clone(),
-            disable_tls,
-            None,
-        ));
+        let rfs = Some(std::rc::Rc::new(std::cell::RefCell::new(
+            RemoteFilesystem::new(
+                io.clone(),
+                config.clone(),
+                options.clone(),
+                disable_tls,
+                None,
+            ),
+        )));
 
         let mut max_jobs: i64 = 12;
         let max_jobs_env = Platform::get_env("COMPOSER_MAX_PARALLEL_HTTP");
@@ -435,21 +437,24 @@ impl HttpDownloader {
         }
 
         let result: anyhow::Result<Response> = {
-            let rfs = self.rfs.as_mut().unwrap();
+            let rfs = self.rfs.as_ref().unwrap().clone();
             (|| -> anyhow::Result<Response> {
                 if let Some(copy_to) = copy_to.as_deref() {
-                    rfs.copy(&origin, &url, copy_to, false, options.clone())?;
+                    let (_, headers) =
+                        rfs.borrow_mut()
+                            .copy(&origin, &url, copy_to, false, options.clone())?;
 
-                    let headers = rfs.get_last_headers().to_vec();
                     let code = RemoteFilesystem::find_status_code(&headers);
                     let body = Some(format!("{}~", copy_to));
                     Ok(Response::new(request.url.clone(), code, headers, body))
                 } else {
-                    let body = match rfs.get_contents(&origin, &url, false, options.clone())? {
+                    let (result, headers) =
+                        rfs.borrow_mut()
+                            .get_contents(&origin, &url, false, options.clone())?;
+                    let body = match result {
                         GetResult::Content(s) => Some(s),
                         _ => None,
                     };
-                    let headers = rfs.get_last_headers().to_vec();
                     let code = RemoteFilesystem::find_status_code(&headers);
                     Ok(Response::new(request.url.clone(), code, headers, body))
                 }
diff --git a/crates/shirabe/src/util/remote_filesystem.rs b/crates/shirabe/src/util/remote_filesystem.rs
index 9b851007..df47be20 100644
--- a/crates/shirabe/src/util/remote_filesystem.rs
+++ b/crates/shirabe/src/util/remote_filesystem.rs
@@ -96,6 +96,9 @@ impl RemoteFilesystem {
         }
     }
 
+    /// Returns the response headers alongside the result (rather than requiring a follow-up
+    /// `get_last_headers()` call) so that a caller sharing this instance via `Rc<RefCell<...>>`
+    /// always reads the headers produced by its own call, not one clobbered by a concurrent one.
     pub fn copy(
         &mut self,
         origin_url: &str,
@@ -103,24 +106,27 @@ impl RemoteFilesystem {
         file_name: &str,
         progress: bool,
         options: IndexMap<String, PhpMixed>,
-    ) -> anyhow::Result<GetResult> {
-        self.get(
+    ) -> anyhow::Result<(GetResult, Vec<String>)> {
+        let result = self.get(
             origin_url,
             file_url,
             options,
             Some(file_name.to_string()),
             progress,
-        )
+        )?;
+        Ok((result, self.last_headers.clone()))
     }
 
+    /// See `copy` for why the headers are bundled into the return value.
     pub fn get_contents(
         &mut self,
         origin_url: &str,
         file_url: &str,
         progress: bool,
         options: IndexMap<String, PhpMixed>,
-    ) -> anyhow::Result<GetResult> {
-        self.get(origin_url, file_url, options, None, progress)
+    ) -> anyhow::Result<(GetResult, Vec<String>)> {
+        let result = self.get(origin_url, file_url, options, None, progress)?;
+        Ok((result, self.last_headers.clone()))
     }
 
     pub fn get_options(&self) -> &IndexMap<String, PhpMixed> {