From 6ce47b20eba3de22e1ee1f04b978d1398878ad06 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Boni=20Garc=C3=ADa?= Date: Tue, 17 Dec 2024 11:45:19 +0100 Subject: [PATCH 1/3] [rust] Use file lock to protect concurrent accesses to cache (fix #13511 and #13686) * [rust] Use file lock to protect concurrent accesses to cache * [rust] Fix several problems related to file manipulation * [rust] Improve logic for handling unstable edge bad links * [rust] Ensure lock file exists and content of target folder after acquiring lock * [rust] Refactor lock logic in a separate module * [rust] Acquire file lock before downloading driver and browser --- rust/Cargo.Bazel.lock | 113 +++++++++++++++++++++++++++++++++++++++++- rust/Cargo.lock | 20 +++++++- rust/Cargo.toml | 2 + rust/src/files.rs | 57 +++++++++++---------- rust/src/lib.rs | 51 +++++++++++++++---- rust/src/lock.rs | 48 ++++++++++++++++++ 6 files changed, 250 insertions(+), 41 deletions(-) create mode 100644 rust/src/lock.rs diff --git a/rust/Cargo.Bazel.lock b/rust/Cargo.Bazel.lock index 57ff8b0722d1f..b50b353ee5967 100644 --- a/rust/Cargo.Bazel.lock +++ b/rust/Cargo.Bazel.lock @@ -1,5 +1,5 @@ { - "checksum": "778a62d50d430548a64ececb679a81afba2ffa2ac1553d2e95807253aa723ac7", + "checksum": "ba73e39704c230ab36d29779338a27060fadfde0a7e9b957428668f5fdb9c271", "crates": { "addr2line 0.21.0": { "name": "addr2line", @@ -4539,6 +4539,100 @@ ], "license_file": "LICENSE-APACHE" }, + "fs2 0.4.3": { + "name": "fs2", + "version": "0.4.3", + "package_url": "https://github.com/danburkert/fs2-rs", + "repository": { + "Http": { + "url": "https://static.crates.io/crates/fs2/0.4.3/download", + "sha256": "9564fc758e15025b46aa6643b1b77d047d1a56a1aea6e01002ac0c7026876213" + } + }, + "targets": [ + { + "Library": { + "crate_name": "fs2", + "crate_root": "src/lib.rs", + "srcs": { + "allow_empty": true, + "include": [ + "**/*.rs" + ] + } + } + } + ], + "library_target_name": "fs2", + "common_attrs": { + "compile_data_glob": [ + "**" + ], + "deps": { + "common": [], + "selects": { + "cfg(unix)": [ + { + "id": "libc 0.2.168", + "target": "libc" + } + ], + "cfg(windows)": [ + { + "id": "winapi 0.3.9", + "target": "winapi" + } + ] + } + }, + "edition": "2015", + "version": "0.4.3" + }, + "license": "MIT/Apache-2.0", + "license_ids": [ + "Apache-2.0", + "MIT" + ], + "license_file": "LICENSE-APACHE" + }, + "fs_extra 1.3.0": { + "name": "fs_extra", + "version": "1.3.0", + "package_url": "https://github.com/webdesus/fs_extra", + "repository": { + "Http": { + "url": "https://static.crates.io/crates/fs_extra/1.3.0/download", + "sha256": "42703706b716c37f96a77aea830392ad231f44c9e9a67872fa5548707e11b11c" + } + }, + "targets": [ + { + "Library": { + "crate_name": "fs_extra", + "crate_root": "src/lib.rs", + "srcs": { + "allow_empty": true, + "include": [ + "**/*.rs" + ] + } + } + } + ], + "library_target_name": "fs_extra", + "common_attrs": { + "compile_data_glob": [ + "**" + ], + "edition": "2018", + "version": "1.3.0" + }, + "license": "MIT", + "license_ids": [ + "MIT" + ], + "license_file": "LICENSE" + }, "futures 0.3.30": { "name": "futures", "version": "0.3.30", @@ -13320,6 +13414,14 @@ "id": "flate2 1.0.35", "target": "flate2" }, + { + "id": "fs2 0.4.3", + "target": "fs2" + }, + { + "id": "fs_extra 1.3.0", + "target": "fs_extra" + }, { "id": "infer 0.16.0", "target": "infer" @@ -18553,7 +18655,12 @@ ], "crate_features": { "common": [ - "winbase" + "fileapi", + "handleapi", + "processthreadsapi", + "std", + "winbase", + "winerror" ], "selects": {} }, @@ -22185,6 +22292,8 @@ "env_logger 0.11.5", "exitcode 1.1.2", "flate2 1.0.35", + "fs2 0.4.3", + "fs_extra 1.3.0", "infer 0.16.0", "log 0.4.22", "regex 1.11.1", diff --git a/rust/Cargo.lock b/rust/Cargo.lock index 94042eaa07204..4fd86b331d003 100644 --- a/rust/Cargo.lock +++ b/rust/Cargo.lock @@ -1,6 +1,6 @@ # This file is automatically @generated by Cargo. # It is not intended for manual editing. -version = 3 +version = 4 [[package]] name = "addr2line" @@ -713,6 +713,22 @@ dependencies = [ "percent-encoding", ] +[[package]] +name = "fs2" +version = "0.4.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9564fc758e15025b46aa6643b1b77d047d1a56a1aea6e01002ac0c7026876213" +dependencies = [ + "libc", + "winapi", +] + +[[package]] +name = "fs_extra" +version = "1.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "42703706b716c37f96a77aea830392ad231f44c9e9a67872fa5548707e11b11c" + [[package]] name = "futures" version = "0.3.30" @@ -1846,6 +1862,8 @@ dependencies = [ "env_logger", "exitcode", "flate2", + "fs2", + "fs_extra", "infer 0.16.0", "is_executable", "log", diff --git a/rust/Cargo.toml b/rust/Cargo.toml index 1c93de992cd05..33fa599154fc5 100644 --- a/rust/Cargo.toml +++ b/rust/Cargo.toml @@ -36,6 +36,8 @@ debpkg = "0.6.0" anyhow = { version = "1.0.94", default-features = false, features = ["backtrace", "std"] } apple-flat-package = "0.20.0" which = "7.0.0" +fs2 = "0.4.3" +fs_extra = "1.3.0" [dev-dependencies] assert_cmd = "2.0.16" diff --git a/rust/src/files.rs b/rust/src/files.rs index bbbf1682d7b0f..8dc208a3cfb85 100644 --- a/rust/src/files.rs +++ b/rust/src/files.rs @@ -27,6 +27,7 @@ use apple_flat_package::PkgReader; use bzip2::read::BzDecoder; use directories::BaseDirs; use flate2::read::GzDecoder; +use fs_extra::dir::{move_dir, CopyOptions}; use regex::Regex; use std::fs; use std::fs::File; @@ -143,7 +144,7 @@ pub fn uncompress( } else if extension.eq_ignore_ascii_case(EXE) { uncompress_sfx(compressed_file, target, log)? } else if extension.eq_ignore_ascii_case(DEB) { - uncompress_deb(compressed_file, target, log, os, volume.unwrap_or_default())? + uncompress_deb(compressed_file, target, log, volume.unwrap_or_default())? } else if extension.eq_ignore_ascii_case(MSI) { install_msi(compressed_file, log, os)? } else if extension.eq_ignore_ascii_case(XML) || extension.eq_ignore_ascii_case(HTML) { @@ -158,6 +159,7 @@ pub fn uncompress( extension ))); } + Ok(()) } @@ -176,15 +178,23 @@ pub fn uncompress_sfx(compressed_file: &str, target: &Path, log: &Logger) -> Res sevenz_rust::decompress(file_reader, zip_parent).unwrap(); let zip_parent_str = path_to_string(zip_parent); - let target_str = path_to_string(target); let core_str = format!(r"{}\core", zip_parent_str); + move_folder_content(&core_str, &target, &log)?; + + Ok(()) +} + +pub fn move_folder_content(source: &str, target: &Path, log: &Logger) -> Result<(), Error> { log.trace(format!( - "Moving extracted files and folders from {} to {}", - core_str, target_str + "Moving files and folders from {} to {}", + source, + target.display() )); create_parent_path_if_not_exists(target)?; - fs::rename(&core_str, &target_str)?; - + let mut options = CopyOptions::new(); + options.content_only = true; + options.skip_exist = true; + move_dir(source, target, &options)?; Ok(()) } @@ -261,7 +271,6 @@ pub fn uncompress_deb( compressed_file: &str, target: &Path, log: &Logger, - os: &str, label: &str, ) -> Result<(), Error> { let zip_parent = Path::new(compressed_file).parent().unwrap(); @@ -276,21 +285,17 @@ pub fn uncompress_deb( deb_pkg.data()?.unpack(zip_parent)?; let zip_parent_str = path_to_string(zip_parent); - let target_str = path_to_string(target); let opt_edge_str = format!("{}/opt/microsoft/{}", zip_parent_str, label); - let opt_edge_mv = format!("mv {} {}", opt_edge_str, target_str); - let command = Command::new_single(opt_edge_mv.clone()); - log.trace(format!( - "Moving extracted files and folders from {} to {}", - opt_edge_str, target_str - )); - create_parent_path_if_not_exists(target)?; - run_shell_command_by_os(os, command)?; - let target_path = Path::new(target); - if target_path.parent().unwrap().read_dir()?.next().is_none() { - fs::rename(&opt_edge_str, &target_str)?; + + // Exception due to bad symbolic link in unstable distributions. For example: + // microsoft-edge -> /opt/microsoft/msedge-beta/microsoft-edge-beta + if !label.eq("msedge") { + let link = format!("{}/microsoft-edge", opt_edge_str); + fs::remove_file(Path::new(&link)).unwrap_or_default(); } + move_folder_content(&opt_edge_str, &target, &log)?; + Ok(()) } @@ -337,14 +342,12 @@ pub fn uncompress_tar(decoder: &mut dyn Read, target: &Path, log: &Logger) -> Re let mut buffer: Vec = Vec::new(); decoder.read_to_end(&mut buffer)?; let mut archive = Archive::new(Cursor::new(buffer)); - if !target.exists() { - for entry in archive.entries()? { - let mut entry_decoder = entry?; - let entry_path: PathBuf = entry_decoder.path()?.iter().skip(1).collect(); - let entry_target = target.join(entry_path); - fs::create_dir_all(entry_target.parent().unwrap())?; - entry_decoder.unpack(entry_target)?; - } + for entry in archive.entries()? { + let mut entry_decoder = entry?; + let entry_path: PathBuf = entry_decoder.path()?.iter().skip(1).collect(); + let entry_target = target.join(entry_path); + fs::create_dir_all(entry_target.parent().unwrap())?; + entry_decoder.unpack(entry_target)?; } Ok(()) } diff --git a/rust/src/lib.rs b/rust/src/lib.rs index 3df601e79bd2c..a2c2411bcb763 100644 --- a/rust/src/lib.rs +++ b/rust/src/lib.rs @@ -21,14 +21,14 @@ use crate::config::{str_to_os, ManagerConfig}; use crate::downloads::download_to_tmp_folder; use crate::edge::{EdgeManager, EDGEDRIVER_NAME, EDGE_NAMES, WEBVIEW2_NAME}; use crate::files::{ - capitalize, collect_files_from_cache, create_parent_path_if_not_exists, - create_path_if_not_exists, default_cache_folder, find_latest_from_cache, get_binary_extension, - path_to_string, + capitalize, collect_files_from_cache, create_path_if_not_exists, default_cache_folder, + find_latest_from_cache, get_binary_extension, path_to_string, }; use crate::files::{parse_version, uncompress, BrowserPath}; use crate::firefox::{FirefoxManager, FIREFOX_NAME, GECKODRIVER_NAME}; use crate::grid::GRID_NAME; use crate::iexplorer::{IExplorerManager, IEDRIVER_NAME, IE_NAMES}; +use crate::lock::Lock; use crate::logger::Logger; use crate::metadata::{ create_browser_metadata, create_stats_metadata, get_browser_version_from_metadata, @@ -59,6 +59,7 @@ pub mod files; pub mod firefox; pub mod grid; pub mod iexplorer; +pub mod lock; pub mod logger; pub mod metadata; pub mod mirror; @@ -184,6 +185,22 @@ pub trait SeleniumManager { // ---------------------------------------------------------- fn download_driver(&mut self) -> Result<(), Error> { + let driver_path_in_cache = self.get_driver_path_in_cache()?; + let driver_name_with_extension = self.get_driver_name_with_extension(); + + let mut lock = Lock::acquire( + &self.get_logger(), + &driver_path_in_cache, + Some(driver_name_with_extension.clone()), + )?; + if !lock.exists() && driver_path_in_cache.exists() { + self.get_logger().debug(format!( + "Driver already in cache: {}", + driver_path_in_cache.display() + )); + return Ok(()); + } + let driver_url = self.get_driver_url()?; self.get_logger().debug(format!( "Downloading {} {} from {}", @@ -196,20 +213,20 @@ pub trait SeleniumManager { if self.is_grid() { let driver_path_in_cache = self.get_driver_path_in_cache()?; - create_parent_path_if_not_exists(&driver_path_in_cache)?; - Ok(fs::rename(driver_zip_file, driver_path_in_cache)?) + fs::rename(driver_zip_file, driver_path_in_cache)?; } else { - let driver_path_in_cache = self.get_driver_path_in_cache()?; - let driver_name_with_extension = self.get_driver_name_with_extension(); - Ok(uncompress( + uncompress( &driver_zip_file, &driver_path_in_cache, self.get_logger(), self.get_os(), Some(driver_name_with_extension), None, - )?) + )?; } + + lock.release(); + Ok(()) } fn download_browser( @@ -304,6 +321,17 @@ pub trait SeleniumManager { ))); } + let browser_path_in_cache = self.get_browser_path_in_cache()?; + let mut lock = Lock::acquire(&self.get_logger(), &browser_path_in_cache, None)?; + if !lock.exists() && browser_binary_path.exists() { + self.get_logger().debug(format!( + "Browser already in cache: {}", + browser_binary_path.display() + )); + self.set_browser_path(path_to_string(&browser_binary_path)); + return Ok(Some(browser_binary_path.clone())); + } + let browser_url = self.get_browser_url_for_download(original_browser_version)?; self.get_logger().debug(format!( "Downloading {} {} from {}", @@ -318,12 +346,13 @@ pub trait SeleniumManager { self.get_browser_label_for_download(original_browser_version)?; uncompress( &driver_zip_file, - &self.get_browser_path_in_cache()?, - self.get_logger(), + &browser_path_in_cache, + &self.get_logger(), self.get_os(), None, browser_label_for_download, )?; + lock.release(); } if browser_binary_path.exists() { self.set_browser_path(path_to_string(&browser_binary_path)); diff --git a/rust/src/lock.rs b/rust/src/lock.rs new file mode 100644 index 0000000000000..dfb987acbf82b --- /dev/null +++ b/rust/src/lock.rs @@ -0,0 +1,48 @@ +use crate::logger::Logger; +use anyhow::Error; +use std::fs::File; +use std::path::{Path, PathBuf}; + +use crate::files::{create_parent_path_if_not_exists, create_path_if_not_exists}; +use fs2::FileExt; +use std::fs; + +const LOCK_FILE: &str = "sm.lock"; + +pub struct Lock { + file: File, + path: PathBuf, +} + +impl Lock { + // Acquire file lock to prevent race conditions accessing the cache folder by concurrent SM processes + pub fn acquire( + log: &Logger, + target: &Path, + single_file: Option, + ) -> Result { + let lock_folder = if single_file.is_some() { + create_parent_path_if_not_exists(target)?; + target.parent().unwrap() + } else { + create_path_if_not_exists(target)?; + target + }; + let path = lock_folder.join(LOCK_FILE); + let file = File::create(&path)?; + + log.debug(format!("Acquiring lock: {}", path.display())); + file.lock_exclusive().unwrap_or_default(); + + Ok(Self { file, path }) + } + + pub fn release(&mut self) { + fs::remove_file(&self.path).unwrap_or_default(); + self.file.unlock().unwrap_or_default(); + } + + pub fn exists(&mut self) -> bool { + self.path.exists() + } +} From cd138fce628b59e2c0c394e07e603385c3bcf30f Mon Sep 17 00:00:00 2001 From: Nikolay Borisenko <22616990+nvborisenko@users.noreply.github.com> Date: Tue, 17 Dec 2024 21:51:45 +0300 Subject: [PATCH 2/3] [dotnet] [cdp] Add more internal logs around CDP implementation Related to #14903 --- dotnet/src/webdriver/DevTools/DevToolsSession.cs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/dotnet/src/webdriver/DevTools/DevToolsSession.cs b/dotnet/src/webdriver/DevTools/DevToolsSession.cs index b58de1097888c..4ec62289873db 100644 --- a/dotnet/src/webdriver/DevTools/DevToolsSession.cs +++ b/dotnet/src/webdriver/DevTools/DevToolsSession.cs @@ -542,6 +542,11 @@ private void MonitorMessageQueue() } catch (Exception ex) { + if (logger.IsEnabled(LogEventLevel.Error)) + { + logger.Error($"Unexpected error occured while processing message: {ex}"); + } + LogError("Unexpected error occured while processing message: {0}", ex); } } @@ -578,6 +583,11 @@ private void ProcessMessage(string message) } else { + if (logger.IsEnabled(LogEventLevel.Error)) + { + logger.Error($"Recieved Unknown Response {commandId}: {message}"); + } + LogError("Recieved Unknown Response {0}: {1}", commandId, message); } From 825b0406e0842992e4dd37c06b2dc9bafa27a37c Mon Sep 17 00:00:00 2001 From: Navin Chandra <98466550+navin772@users.noreply.github.com> Date: Thu, 19 Dec 2024 14:31:01 +0530 Subject: [PATCH 3/3] [java]: Improved span name for `TracedCommandExecutor` (#14902) Co-authored-by: Puja Jagani --- .../selenium/remote/TracedCommandExecutor.java | 2 +- .../remote/TracedCommandExecutorTest.java | 17 ++++++++++++++++- 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/java/src/org/openqa/selenium/remote/TracedCommandExecutor.java b/java/src/org/openqa/selenium/remote/TracedCommandExecutor.java index a8d0b220ea678..a33d52d79e758 100644 --- a/java/src/org/openqa/selenium/remote/TracedCommandExecutor.java +++ b/java/src/org/openqa/selenium/remote/TracedCommandExecutor.java @@ -37,7 +37,7 @@ public TracedCommandExecutor(CommandExecutor delegate, Tracer tracer) { @Override public Response execute(Command command) throws IOException { - try (Span commandSpan = tracer.getCurrentContext().createSpan("command")) { + try (Span commandSpan = tracer.getCurrentContext().createSpan(command.getName())) { SessionId sessionId = command.getSessionId(); if (sessionId != null) { commandSpan.setAttribute("sessionId", sessionId.toString()); diff --git a/java/test/org/openqa/selenium/remote/TracedCommandExecutorTest.java b/java/test/org/openqa/selenium/remote/TracedCommandExecutorTest.java index e747b712092fc..6a7aa226826f4 100644 --- a/java/test/org/openqa/selenium/remote/TracedCommandExecutorTest.java +++ b/java/test/org/openqa/selenium/remote/TracedCommandExecutorTest.java @@ -17,6 +17,7 @@ package org.openqa.selenium.remote; +import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoMoreInteractions; @@ -48,7 +49,7 @@ class TracedCommandExecutorTest { public void createMocksAndTracedCommandExecutor() { MockitoAnnotations.initMocks(this); when(tracer.getCurrentContext()).thenReturn(traceContext); - when(traceContext.createSpan("command")).thenReturn(span); + when(traceContext.createSpan(anyString())).thenReturn(span); tracedCommandExecutor = new TracedCommandExecutor(commandExecutor, tracer); } @@ -109,4 +110,18 @@ void canCreateSpanWithCommandName() throws IOException { verify(span, times(1)).close(); verifyNoMoreInteractions(span); } + + @Test + void canCreateSpanWithCommandNameAsSpanName() throws IOException { + SessionId sessionId = new SessionId(UUID.randomUUID()); + Command command = new Command(sessionId, "findElement"); + + tracedCommandExecutor.execute(command); + + verify(traceContext).createSpan("findElement"); + verify(span).setAttribute("sessionId", sessionId.toString()); + verify(span).setAttribute("command", "findElement"); + verify(span).close(); + verifyNoMoreInteractions(span); + } }