Skip to content

[java] Synchronize method to get Selenium Manager binary (fix #11620)#11640

Merged
diemol merged 1 commit intotrunkfrom
se_mgr_java_parallel
Feb 11, 2023
Merged

[java] Synchronize method to get Selenium Manager binary (fix #11620)#11640
diemol merged 1 commit intotrunkfrom
se_mgr_java_parallel

Conversation

@bonigarcia
Copy link
Copy Markdown
Member

Description

The method to get the Selenium Manager binary in the Java bindings is not synchronized, which leading to concurrent problems to get Selenium Manager in Java, as reported in #11620. I have confirmed the bug using the provided example repo. I used that repo as well to test the fix.

Motivation and Context

This PR fixes #11620.

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.

@krmahadevan
Copy link
Copy Markdown
Contributor

@bonigarcia - I think there's a larger problem here.

  • org.openqa.selenium.manager.SeleniumManager has been built to be a singleton.
  • The current implementation (as in 4.8.0) right now is such that, it attempts to locate the binary data member ONLY once. This means that lets say I have 2 test methods say firefoxTest() and chromeTest() then the following will happen if I run them in parallel

Assumptions (Only this will kick in the SeleniumManager into action)

  • chromedriver and geckodriver are NOT available in my PATH

  • I am not specifying the location of chromedriver and geckodriver via the JVM arguments.

  • firefoxTest() will cause the binary data member in the SeleniumManager singleton to be set.

  • Now because of this edit check we will end up returning the path of geckodriver to chromeTest This will be wrong.

I think the fix should be to do something like below (I have changed SeleniumManager to be an enum since enums are first class singletons)

public enum SeleniumManager {
    manager;
    private static final Logger LOG = Logger.getLogger(SeleniumManager.class.getName());

    private static final String SELENIUM_MANAGER = "selenium-manager";
    private static final String EXE = ".exe";
    private static final String INFO = "INFO\t";

    //We cache the binary path on a per driver flavor basis.
    private final Map<String, File> binaries = new ConcurrentHashMap<>();

    SeleniumManager() {
        Runtime.getRuntime().addShutdownHook(new Thread(() -> binaries.values()
          .stream()
          .filter(Objects::nonNull)
          .filter(File::exists)
          .forEach(SeleniumManager::deleteQuietly)));
    }

  private static void deleteQuietly(File binary) {
    try {
      Files.delete(binary.toPath());
    } catch (IOException e) {
      LOG.warning(String.format("%s deleting temporal file: %s",
        e.getClass().getSimpleName(), e.getMessage()));
    }

  }

    public SeleniumManager getInstance() {
        return manager;
    }

  private File getBinary(String driverName) {
    return this.binaries.computeIfAbsent(driverName, flavor -> {
      try {
        Platform current = Platform.getCurrent();
        String folder = "linux";
        String extension = "";
        if (current.is(WINDOWS)) {
          extension = EXE;
          folder = "windows";
        } else if (current.is(MAC)) {
          folder = "macos";
        }
        String binaryPath = String.format("%s/%s%s", folder, SELENIUM_MANAGER, extension);
        try (InputStream inputStream = this.getClass().getResourceAsStream(binaryPath)) {
          Path tmpPath = Files.createTempDirectory(SELENIUM_MANAGER + System.nanoTime());
          File tmpFolder = tmpPath.toFile();
          tmpFolder.deleteOnExit();
          File binary = new File(tmpFolder, SELENIUM_MANAGER + extension);
          Files.copy(inputStream, binary.toPath(), REPLACE_EXISTING);
          binary.setExecutable(true);
          return binary;
        }
      } catch (Exception e) {
        throw new WebDriverException("Unable to obtain Selenium Manager", e);
      }
    });
  }

    public String getDriverPath(String driverName) {
        if (!ImmutableList.of("geckodriver", "chromedriver", "msedgedriver", "IEDriverServer").contains(driverName)) {
            throw new WebDriverException("Unable to locate driver with name: " + driverName);
        }

        String driverPath = null;
        File binaryFile = getBinary(driverName);
        if (binaryFile != null) {
          driverPath = runCommand(binaryFile.getAbsolutePath(),
                    "--driver", driverName.replaceAll(EXE, ""));
        }
        return driverPath;
    }
}

@bonigarcia
Copy link
Copy Markdown
Member Author

  • Now because of this edit check we will end up returning the path of geckodriver to chromeTest This will be wrong.

That is wrong. That binary path is the Selenium Manager binary, not the driver. The Selenium Manager binary is used to manage (i.e., download, cache, etc.) the drivers (geckodriver, chromedriver, etc.).

@krmahadevan
Copy link
Copy Markdown
Contributor

  • Now because of this edit check we will end up returning the path of geckodriver to chromeTest This will be wrong.

That is wrong. That binary path is the Selenium Manager binary, not the driver. The Selenium Manager binary is used to manage (i.e., download, cache, etc.) the drivers (geckodriver, chromedriver, etc.).

Ah! Yeah now I realise my blunder. The SeleniumManager internally relies on the rust binary to do the actual download. Sorry for the confusion @bonigarcia

@titusfortner
Copy link
Copy Markdown
Member

Is the issue getting the binary or executing the binary? I'm a little confused because of the singleton; does synchronizing that method actually prevent two threads from attempting to execute the binary at the same time? When does the first thread relinquish the lock?

Copy link
Copy Markdown
Member

@diemol diemol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Failing tests are not related to this change.

@diemol diemol merged commit 1e2a4c4 into trunk Feb 11, 2023
@diemol diemol deleted the se_mgr_java_parallel branch February 11, 2023 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[🐛 Bug]: Selenium Manager sometimes fails in parallel test run

4 participants