From 9b291d454b059977639db26c847af4e835cda0c3 Mon Sep 17 00:00:00 2001 From: nsfisis Date: Sat, 18 Jul 2026 18:03:57 +0900 Subject: refactor(downloader): take &self across the downloader hierarchy Concurrent package operations call into the same downloader instances through Rc>; with &mut self methods every call holds a RefMut across its awaits, which panics with 'already mutably borrowed' the moment two operations overlap. This is groundwork for fanning out InstallationManager's download/install loops (same rework HttpDownloader/CurlDownloader already got). - DownloaderInterface/ChangeReportInterface/ArchiveDownloader/ VcsDownloader methods now take &self; as_change_report_interface returns &dyn instead of &mut dyn. - Implementors move their genuinely mutable state behind cells: FileDownloader.additional_cleanup_paths, the archive downloaders' cleanup_executed, ZipDownloader.zip_archive_object, VcsDownloaderBase.has_cleaned_changes, GitDownloader's stash/discard/ cache maps and GitUtil, SvnDownloader.cache_credentials, PerforceDownloader.perforce. FileDownloader.io gains a RefCell layer so get_local_changes can keep PHP's NullIO swap under &self. - ProcessExecutor::execute_async now returns a future that captures everything up front instead of borrowing the executor, and call sites build the future before awaiting, so no borrow on the shared executor is held while a subprocess runs. - Filesystem::remove_directory_async becomes remove_directory_async_via taking the Rc handle: the Filesystem is only borrowed for the sync head/tail, never across the rm subprocess await (sync borrow_mut users like rename/ensure_directory_exists would otherwise collide). - DownloadManager async call sites hold shared borrows only. Co-Authored-By: Claude Fable 5 --- crates/shirabe/src/downloader/file_downloader.rs | 79 +++++++++++++----------- 1 file changed, 43 insertions(+), 36 deletions(-) (limited to 'crates/shirabe/src/downloader/file_downloader.rs') diff --git a/crates/shirabe/src/downloader/file_downloader.rs b/crates/shirabe/src/downloader/file_downloader.rs index 6286918a..9742fd70 100644 --- a/crates/shirabe/src/downloader/file_downloader.rs +++ b/crates/shirabe/src/downloader/file_downloader.rs @@ -51,7 +51,10 @@ pub static RESPONSE_HEADERS: LazyLock>>> = #[derive(Debug)] pub struct FileDownloader { /// @var IOInterface - pub(crate) io: std::rc::Rc>, + /// + /// Behind a RefCell so `get_local_changes` can temporarily swap in a NullIO through `&self` + /// (PHP: `$this->io = new NullIO;` ... restore), now that the downloader methods take `&self`. + pub(crate) io: std::cell::RefCell>>, /// @var Config pub(crate) config: std::rc::Rc>, /// @var HttpDownloader @@ -72,7 +75,10 @@ pub struct FileDownloader { /// this write needs guarding, so it is the only field isolated behind a lock. last_cache_writes: Mutex>, /// @var array Map of package name to list of paths - additional_cleanup_paths: IndexMap>, + /// + /// Behind a RefCell so add/remove_cleanup_path work through `&self` (needed since install() + /// mutates it while downloads of other packages may be in flight on sibling futures). + additional_cleanup_paths: std::cell::RefCell>>, } impl FileDownloader { @@ -106,7 +112,7 @@ impl FileDownloader { }); let this = Self { - io, + io: std::cell::RefCell::new(io), config, http_downloader, event_dispatcher, @@ -114,14 +120,14 @@ impl FileDownloader { process, filesystem, last_cache_writes: Mutex::new(IndexMap::new()), - additional_cleanup_paths: IndexMap::new(), + additional_cleanup_paths: std::cell::RefCell::new(IndexMap::new()), }; if let Some(cache) = &this.cache && cache.borrow().gc_is_necessary() { // PHP: writeError('Running cache garbage collection', true, io_interface::VERY_VERBOSE) - this.io.write_error("Running cache garbage collection"); + this.io.borrow().write_error("Running cache garbage collection"); let ttl = this .config .borrow_mut() @@ -148,15 +154,13 @@ impl DownloaderInterface for FileDownloader { "dist".to_owned() } - fn as_change_report_interface( - &mut self, - ) -> Option<&mut dyn crate::downloader::ChangeReportInterface> { + fn as_change_report_interface(&self) -> Option<&dyn crate::downloader::ChangeReportInterface> { Some(self) } /// @inheritDoc async fn download( - &mut self, + &self, package: PackageInterfaceHandle, path: &str, _prev_package: Option, @@ -234,7 +238,7 @@ impl DownloaderInterface for FileDownloader { if from_cache { if output { - self.io.write_error3( + self.io.borrow().write_error3( &format!( " - Loading {} ({}) from cache", package.get_name(), @@ -260,7 +264,7 @@ impl DownloaderInterface for FileDownloader { } } else { if output { - self.io.write_error(&format!( + self.io.borrow().write_error(&format!( " - Downloading {} ({})", package.get_name(), package.get_full_pretty_version( @@ -272,7 +276,7 @@ impl DownloaderInterface for FileDownloader { let add_copy_result = self .http_downloader - .borrow_mut() + .borrow() .add_copy(&url.processed, &file_name, package.get_transport_options()) .await; match add_copy_result { @@ -367,20 +371,20 @@ impl DownloaderInterface for FileDownloader { let code = e .downcast_ref::() .map_or(0, |te| te.get_code()); - if self.io.is_debug() { - self.io.write_error(&format!( + if self.io.borrow().is_debug() { + self.io.borrow().write_error(&format!( " Failed downloading {}: [{}] {}: {}", package.get_name(), get_class(&PhpMixed::Null), code, e )); - self.io.write_error(&format!( + self.io.borrow().write_error(&format!( " Trying the next URL for {}", package.get_name() )); } else { - self.io.write_error(&format!( + self.io.borrow().write_error(&format!( " Failed downloading {}, trying the next URL ({}: {})", package.get_name(), code, @@ -433,7 +437,7 @@ impl DownloaderInterface for FileDownloader { /// @inheritDoc async fn prepare( - &mut self, + &self, _type: &str, _package: PackageInterfaceHandle, _path: &str, @@ -444,7 +448,7 @@ impl DownloaderInterface for FileDownloader { /// @inheritDoc async fn cleanup( - &mut self, + &self, _type: &str, package: PackageInterfaceHandle, path: &str, @@ -477,6 +481,7 @@ impl DownloaderInterface for FileDownloader { if let Some(paths) = self .additional_cleanup_paths + .borrow() .get(&package.get_name()) .cloned() { @@ -499,13 +504,13 @@ impl DownloaderInterface for FileDownloader { /// @inheritDoc async fn install( - &mut self, + &self, package: PackageInterfaceHandle, path: &str, output: bool, ) -> anyhow::Result> { if output { - self.io.write_error(&format!( + self.io.borrow().write_error(&format!( " - {}", InstallOperation::format(package.clone(), false) )); @@ -556,12 +561,12 @@ impl DownloaderInterface for FileDownloader { /// @inheritDoc async fn update( - &mut self, + &self, initial: PackageInterfaceHandle, target: PackageInterfaceHandle, path: &str, ) -> anyhow::Result> { - self.io.write_error(&format!( + self.io.borrow().write_error(&format!( " - {}{}", UpdateOperation::format(initial.clone(), target.clone(), false), self.get_install_operation_appendix(target.clone(), path) @@ -574,22 +579,18 @@ impl DownloaderInterface for FileDownloader { /// @inheritDoc async fn remove( - &mut self, + &self, package: PackageInterfaceHandle, path: &str, output: bool, ) -> anyhow::Result> { if output { - self.io.write_error(&format!( + self.io.borrow().write_error(&format!( " - {}", UninstallOperation::format(package, false) )); } - let result = self - .filesystem - .borrow_mut() - .remove_directory_async(path) - .await?; + let result = Filesystem::remove_directory_async_via(&self.filesystem, path).await?; if !result { return Err(RuntimeException { message: format!("Could not completely delete {}, aborting.", path), @@ -606,15 +607,16 @@ impl ChangeReportInterface for FileDownloader { /// @inheritDoc /// @throws \RuntimeException fn get_local_changes( - &mut self, + &self, package: PackageInterfaceHandle, path: &str, ) -> anyhow::Result> { let prev_io = std::mem::replace( - &mut self.io, + &mut *self.io.borrow_mut(), std::rc::Rc::new(std::cell::RefCell::new(NullIO::new())), ); self.io + .borrow() .borrow_mut() .load_configuration(&mut self.config.borrow_mut())?; @@ -652,7 +654,7 @@ impl ChangeReportInterface for FileDownloader { Ok(output) })(); - self.io = prev_io; + *self.io.borrow_mut() = prev_io; let (e, output) = match result { Ok(output) => (None, output), @@ -660,7 +662,7 @@ impl ChangeReportInterface for FileDownloader { }; if let Some(err) = e { - if self.io.is_debug() { + if self.io.borrow().is_debug() { return Err(err); } @@ -712,15 +714,20 @@ impl FileDownloader { } } - pub(crate) fn add_cleanup_path(&mut self, package: PackageInterfaceHandle, path: &str) { + pub(crate) fn add_cleanup_path(&self, package: PackageInterfaceHandle, path: &str) { self.additional_cleanup_paths + .borrow_mut() .entry(package.get_name()) .or_default() .push(path.to_string()); } - pub(crate) fn remove_cleanup_path(&mut self, package: PackageInterfaceHandle, path: &str) { - if let Some(paths) = self.additional_cleanup_paths.get_mut(&package.get_name()) { + pub(crate) fn remove_cleanup_path(&self, package: PackageInterfaceHandle, path: &str) { + if let Some(paths) = self + .additional_cleanup_paths + .borrow_mut() + .get_mut(&package.get_name()) + { // PHP: array_search($path, ..., true) let idx = paths.iter().position(|p| p == path); if let Some(i) = idx { -- cgit v1.3.1