-
Notifications
You must be signed in to change notification settings - Fork 252
fix: Replaced classLoader in DriverJar #1811
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
Conversation
Since we could not reproduce it before, can you add a test that demonstrates the bug and that the fix actually resolves it? |
Ok, I will try it |
Hi, @yury-s However, when running the same code inside a Docker container, the following issue occurs: Thread.currentThread().getContextClassLoader() returns null
This causes failures when attempting to load resources (e.g., getResource(...)). After investigation, I found that switching to: this.getClass().getClassLoader()
resolves the issue both locally and in the Docker container. To help reproduce and compare the issue, I’ve created a sample repo here: You can build the Docker image using the included Dockerfile and test the endpoints: http://localhost:8080/playwright?url=https://google.com (uses Thread.currentThread().getContextClassLoader()) -> You can check the error log in the console when accessing this endpoint. http://localhost:8080/playwright2?url=https://google.com (uses this.getClass().getClassLoader()) This is my first open source contribution, so I’d appreciate any feedback or suggestions! Thank you 🙇 |
The test has quite some logic which make it not obvious wether the problem is in the test or in Playwright. Can you try to reduce it to a minimal example and drop all unrelated logic? Ideally it would be converted into one of the Playwright tests. If the setup requires a custom environment (e.g. spring framework), you can put it in its own project. There is already a bunch of custom tests in the tools/ directory, particularly this one. You could convert yours into a similar one. We also run Docker tests for our container on GitHub Actions: https://github.com/microsoft/playwright-java/actions/workflows/test_docker.yml. If the test requires Docker environment, we can run the new test inside the Playwright Docker image as part of the same test suite. Would that work? Given that this PR change the core part of the project, I'd really like to have a test that covers it. |
Sure, I’ll go ahead with that. |
Hi, @yury-s The test consists of two approaches: To verify the behavior in the Docker environment, I followed the test_docker.yml setup: CONTAINER_ID="$(docker run --rm -e CI --ipc=host -v $(pwd):/root/playwright --name playwright-docker-test -d -t playwright-java:localbuild-noble /bin/bash)" java -jar /mypath/playwright/tools/test-spring-classloader/target/test-spring-classloader-1.50.0-SNAPSHOT.jar As a result, the first approach failed with an error, while the second one succeeded, confirming the issue. |
@microsoft-github-policy-service agree |
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.
Since we are testing the behavior of our default DriverJar implementation let's drop this class. If the test fails before the change in DriverJar.java and starts passing after, it's good enough.
} | ||
|
||
public void run(String... args) { | ||
System.out.println("Starting original Playwright test..."); |
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.
We don't need to test original Playwright.
|
||
CompletableFuture<Void> voidCompletableFuture2 = CompletableFuture.runAsync(() -> { | ||
try (Playwright playwright = Playwright.create()) { | ||
System.out.println("new Playwright test started, waiting for completion..."); |
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.
How is this test different from the existing ? Can we just extend that one?
From what I understand, the test only fails in a specific docker environment, so can you add it to the corresponding CI step, similar to this ?
If it fails only in the Docker environment, you can run it as part of the existing Docker tests
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.
Understood. I’ll proceed accordingly.
Hi, @yury-s I’ve added the necessary steps to the Docker tests and added the script to package and run tests using shell file |
same exception meets when i use playwright to convert html2pdf in docker environment. |
SpringApplication.run(TestApp.class, args); | ||
} | ||
|
||
public void run(String... args) { |
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.
Can we just run existing test-spring-boot-starter
in Docker? It appears to do the same as the new test, so I'd just put the new shell script in test-spring-boot-starter
and call it from test_docker.yml
. Would that work or am I missing something?
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.
The key difference is that the new test uses CompletableFuture for asynchronous execution. While the existing test works fine even in the Docker environment, i encountered an issue where DriverJar could not be read when executed from a new thread created by CompletableFuture inside the Docker container. That’s why this additional test was introduced.
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.
In that case you let's just modify existing test to either always run playwright asynchronously or, if you want to keep testing sync code path as well, do it sync/async based on a command line flag and pass the flag only when running in docker. Something like this:
public static void main(String[] args) {
if ("--async".equals(args[0])) {
CompletableFuture<Void> voidCompletableFuture = CompletableFuture.runAsync(() -> {
SpringApplication.run(TestApp.class, args);
});
voidCompletableFuture.join();
} else {
SpringApplication.run(TestApp.class, args);
}
}
Would that work?
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.
When running with --async, Spring Boot uses the main thread’s context class loader to load auto-configuration classes. However, in the asynchronous thread, this context class loader is not correctly inherited, which causes it to fail to locate the configuration files.
This results in the following error: java.lang.IllegalArgumentException: No auto configuration classes found in META-INF/spring.factories.
11:48:21.637 [ForkJoinPool.commonPool-worker-1] ERROR org.springframework.boot.SpringApplication - Application run failed
java.lang.IllegalArgumentException: No auto configuration classes found in META-INF/spring.factories. If you are using a custom packaging, make sure that file is correct.
at org.springframework.util.Assert.notEmpty(Assert.java:470)
at org.springframework.boot.autoconfigure.AutoConfigurationImportSelector.getCandidateConfigurations(AutoConfigurationImportSelector.java:180)
at org.springframework.boot.autoconfigure.AutoConfigurationImportSelector.getAutoConfigurationEntry(AutoConfigurationImportSelector.java:123)
at org.springframework.boot.autoconfigure.AutoConfigurationImportSelector$AutoConfigurationGroup.process(AutoConfigurationImportSelector.java:434)
at org.springframework.context.annotation.ConfigurationClassParser$DeferredImportSelectorGrouping.getImports(ConfigurationClassParser.java:879)
at org.springframework.context.annotation.ConfigurationClassParser$DeferredImportSelectorGroupingHandler.processGroupImports(ConfigurationClassParser.java:809)
at org.springframework.context.annotation.ConfigurationClassParser$DeferredImportSelectorHandler.process(ConfigurationClassParser.java:780)
at org.springframework.context.annotation.ConfigurationClassParser.parse(ConfigurationClassParser.java:193)
at org.springframework.context.annotation.ConfigurationClassPostProcessor.processConfigBeanDefinitions(ConfigurationClassPostProcessor.java:330)
at org.springframework.context.annotation.ConfigurationClassPostProcessor.postProcessBeanDefinitionRegistry(ConfigurationClassPostProcessor.java:246)
at org.springframework.context.support.PostProcessorRegistrationDelegate.invokeBeanDefinitionRegistryPostProcessors(PostProcessorRegistrationDelegate.java:311)
at org.springframework.context.support.PostProcessorRegistrationDelegate.invokeBeanFactoryPostProcessors(PostProcessorRegistrationDelegate.java:112)
at org.springframework.context.support.AbstractApplicationContext.invokeBeanFactoryPostProcessors(AbstractApplicationContext.java:745)
at org.springframework.context.support.AbstractApplicationContext.refresh(AbstractApplicationContext.java:563)
at org.springframework.boot.SpringApplication.refresh(SpringApplication.java:767)
at org.springframework.boot.SpringApplication.refresh(SpringApplication.java:759)
at org.springframework.boot.SpringApplication.refreshContext(SpringApplication.java:426)
at org.springframework.boot.SpringApplication.run(SpringApplication.java:326)
at org.springframework.boot.SpringApplication.run(SpringApplication.java:1311)
at org.springframework.boot.SpringApplication.run(SpringApplication.java:1300)
at com.microsoft.playwright.springboottest.TestApp.lambda$main$0(TestApp.java:16)
at java.base/java.util.concurrent.CompletableFuture$AsyncRun.run(CompletableFuture.java:1804)
at java.base/java.util.concurrent.CompletableFuture$AsyncRun.exec(CompletableFuture.java:1796)
at java.base/java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:387)
at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1312)
at java.base/java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:1843)
at java.base/java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1808)
at java.base/java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:188)
While it’s possible to manually pass the main thread’s context class loader to the async thread like below, doing so defeats the original purpose of testing the class loader behavior.
public static void main(String[] args) {
if (args.length == 0) {
SpringApplication.run(TestApp.class, args);
}
else {
if ("--async".equals(args[0])) {
ClassLoader contextClassLoader = Thread.currentThread().getContextClassLoader();
CompletableFuture<Void> voidCompletableFuture = CompletableFuture.runAsync(() -> {
Thread.currentThread().setContextClassLoader(contextClassLoader);
SpringApplication.run(TestApp.class, args);
});
voidCompletableFuture.join();
}
}
}
For this reason, I think separating the test into a different package would make the intention clearer.
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.
Sorry, I meant to just share the implementation of the actual test method, I guess it should have been something like this:
@Override
public void run(String... args) {
if ("--async".equals(args[0])) {
runAsync();
} else {
runSync();
}
}
private void runAsync() {
CompletableFuture<Void> voidCompletableFuture = CompletableFuture.runAsync(() -> {
runSync();
});
voidCompletableFuture.join();
}
private void runSync() {
try (Playwright playwright = Playwright.create()) {
BrowserType browserType = getBrowserTypeFromEnv(playwright);
System.out.println("Running test with " + browserType.name());
Browser browser = browserType.launch();
BrowserContext context = browser.newContext();
Page page = context.newPage();
System.out.println(page.evaluate("'SUCCESS: did evaluate in page'"));
}
}
Basically the logic from the existing test is in runSync()
and reused. I don't suggest we make any changes to the class loaders.
Unrelated, if you update on the latest |
if (args.length == 0) { | ||
runSync(); | ||
} else { | ||
if ("--async".equals(args[0])) { | ||
runAsync(); | ||
} | ||
else { | ||
runSync(); | ||
} | ||
} |
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.
I modified the if statement to prevent an IndexOutOfBoundsException.
Hi, @yury-s |
|
||
@Override | ||
public void run(String... args) { | ||
if (args.length == 0) { |
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.
How about the following, so that each version is called from exactly one branch?
if (Arrays.asList(args).contains("--async")) {
runAsync();
} else {
runSync();
}
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.
That’s a great suggestion.
I’ve updated the implementation accordingly and committed the changes.
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.
Thanks for contributing this and for your patience addressing all the review comments!
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.
I’m glad I had the opportunity to contribute. Since this was my first open source contribution, I struggled quite a bit, but your reviews and comments were a huge help. Thank you!
.github/workflows/test_docker.yml
Outdated
- name: Test | ||
run: | | ||
CONTAINER_ID="$(docker run --rm -e CI --ipc=host -v $(pwd):/root/playwright --name playwright-docker-test -d -t playwright-java:localbuild-${{ matrix.flavor }} /bin/bash)" | ||
docker exec "${CONTAINER_ID}" /root/playwright/tools/test-local-installation/create_project_and_run_tests.sh |
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.
To avoid the docker test failures let's just add docker stop "${CONTAINER_ID}"
to this (and the new) step to shutdown the container after the test has finished. Or better yet, since the container is the same in both steps, you can move its creation in a separate step and then use its CONTAINER_ID without relaunching, something like
- name: Start container
run: |
CONTAINER_ID=$(docker run --rm -e CI --ipc=host -v "$(pwd)":/root/playwright --name playwright-docker-test -d -t playwright-java:localbuild-${{ matrix.flavor }} /bin/bash)
echo "CONTAINER_ID=$CONTAINER_ID" >> $GITHUB_ENV
- name: Run test in container
run: |
docker exec "$CONTAINER_ID" /root/playwright/tools/test-local-installation/create_project_and_run_tests.sh
- name: Test ClassLoader
run: |
docker exec "${CONTAINER_ID}" /root/playwright/tools/test-spring-boot-starter/package_and_run_async_test.sh
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.
I added example code and stop step
.github/workflows/test_docker.yml
Outdated
- name: Test ClassLoader | ||
run: | | ||
docker exec "${CONTAINER_ID}" chmod +x /root/playwright/tools/test-spring-boot-starter/package_and_run_async_test.sh |
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.
I added a permission grant command to the workflow to resolve the permission issue.
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.
Just add executable bit to package_and_run_async_test.sh
similar to create_project_and_run_tests.sh
file above? It should be committed with the executable bit to the repo and you won't need to run this command every time.
…nd_run_async_test.sh file
Hi, @yury-s |
@yury-s
In some environments, such as Docker or the sbt plugin, accessing the ContextClassLoader is not reliable.
Because the driver package is bundled within the JAR, class.getClassLoader() is used instead.
issues: #1447 , #1803
Fixes #1803