-
-
Notifications
You must be signed in to change notification settings - Fork 8.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[java] Synchronize method to get Selenium Manager binary (fix #11620) #11640
Conversation
@bonigarcia - I think there's a larger problem here.
Assumptions (Only this will kick in the SeleniumManager into action)
I think the fix should be to do something like below (I have changed 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;
}
} |
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 |
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? |
There was a problem hiding this 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.
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
Checklist