[java] SpotBugs exclude IS2_INCONSISTENT_SYNC form the SeleniumManager

[mk868]

User description

Description

As described in this comment, SpotBugs found some additional bugs in the code.
In this PR I fix part of the found problems.

SpotBugs error:

Executing tests from //java/src/org/openqa/selenium/manager:manager-lib-spotbugs
-----------------------------------------------------------------------------
M M IS: Inconsistent synchronization of org.openqa.selenium.manager.SeleniumManager.binary; locked 71% of time  Unsynchronized access at SeleniumManager.java:[line 87]

documentation

Solution:

Exclude this SpotBugs check, I think this is not a problem for the code that is in the ShutdownHook.

Alternative solution

We can synchronize to the object to read the binary variable:

diff --git a/java/src/org/openqa/selenium/manager/SeleniumManager.java b/java/src/org/openqa/selenium/manager/SeleniumManager.java
index b907d8ab75..803c4c4cab 100644
--- a/java/src/org/openqa/selenium/manager/SeleniumManager.java
+++ b/java/src/org/openqa/selenium/manager/SeleniumManager.java
@@ -73,6 +73,10 @@ public class SeleniumManager {
   private final String seleniumManagerVersion;
   private boolean binaryInTemporalFolder = false;
 
+  private synchronized Path getBinarySync() {
+    return binary;
+  }
+
   /** Wrapper for the Selenium Manager binary. */
   private SeleniumManager() {
     BuildInfo info = new BuildInfo();
@@ -84,9 +88,10 @@ private SeleniumManager() {
           .addShutdownHook(
               new Thread(
                   () -> {
-                    if (binaryInTemporalFolder && binary != null && Files.exists(binary)) {
+                    final Path finalBinary = getBinarySync();
+                    if (binaryInTemporalFolder && finalBinary != null && Files.exists(finalBinary)) {
                       try {
-                        Files.delete(binary);
+                        Files.delete(finalBinary);
                       } catch (IOException e) {
                         LOG.warning(
                             String.format(

However, I think this is unnecessary and potentially dangerous in the ShutdownHook (I imagine there may be a deadlock on the SeleniumManager.this object).

Motivation and Context

Fixing the actual problems is necessary before enabling full SpotBugs analysis, in order to not break the build.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• I have read the contributing document.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.

---

PR Type

bug_fix, enhancement

---

Description

• Excluded the IS2_INCONSISTENT_SYNC pattern from SpotBugs checks for the SeleniumManager class to suppress warnings about inconsistent synchronization.
• This change addresses a SpotBugs error related to unsynchronized access in the SeleniumManager class.

---

Changes walkthrough 📝

	Relevant files
Bug fix	spotbugs-excludes.xml Exclude IS2_INCONSISTENT_SYNC from SpotBugs checks in SeleniumManager java/spotbugs-excludes.xml Added IS2_INCONSISTENT_SYNC to the list of excluded SpotBugs patterns for SeleniumManager. This change suppresses the inconsistent synchronization warning. +1/-1

---

💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

[qodo-merge-pro]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Code Quality Suppressing the IS2_INCONSISTENT_SYNC warning might hide potential thread safety issues in SeleniumManager. Consider implementing proper synchronization instead of excluding the warning.

[qodo-merge-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Score
Possible issue	Suppressing synchronization warnings may hide potential thread-safety issues that should be addressed in the code Instead of suppressing the IS2_INCONSISTENT_SYNC warning, consider fixing the underlying synchronization issue in SeleniumManager by properly synchronizing access to shared mutable fields. java/spotbugs-excludes.xml [233-236] <Match> <Class name="org.openqa.selenium.manager.SeleniumManager"/> - <Bug pattern="NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE,IS2_INCONSISTENT_SYNC"/> + <Bug pattern="NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE"/> </Match> Apply this suggestion Suggestion importance[1-10]: 8 Why: The suggestion correctly identifies a potential thread-safety concern. Instead of suppressing the synchronization warning, addressing the underlying thread-safety issue in SeleniumManager would be more beneficial for code reliability and correctness.	8

💡 Need additional feedback ? start a PR chat

[mk868]

FYI, This is the last PR addressing SpotBugs problems, in fact there were more problems than I described in the linked comment.