-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Test container fails to stop when used as Spring bean #369
Comments
Hmm, presumably there's nothing calling I'll try and have a better look tomorrow - I think we should be able to do something, it just might mean working out where the right hook point is to force a proper shutdown. |
Thanks for the feedback @rnorth! Well, with @Bean(initMethod = "start", destroyMethod = "stop")
public GenericContainer redisContainer() {
return new GenericContainer("redis:3.2.9").withExposedPorts(6379);
} |
I've discovered something which might help in pinpointing the actual bug. After changing the project to use an embedded Tomcat, an error will be thrown on shutdown:
This seems to be a race condition between the Spring Context calling the destroy method and the testcontainers-java/core/src/main/java/org/testcontainers/utility/ResourceReaper.java Line 34 in 8c14b99
I could reproduce the error when building a fat-jar with |
I've started to debug a litte more into the Spring code and I found, that
afterwards, so maybe we have to look deeper into Spring to find the actual reason? |
Thanks for the feedback @kiview, we'll try to get a better look at Spring part of this. |
@vpavic When the test is being executed by JUnit, it calls System.exit(0) at the end of all tests. It makes interrupts all threads, including non-daemon ones. When you run your app with TC inside some application server, it doesn't happen and for instance, Netty keeps its threads. I'll look at it and will try to solve by providing additional API to "close" everything in TestContainers :) |
@vpavic I'm working on it right now, but can't promise anything at this moment :) So far it seems that the problem might come from docker-java and not TC itself. If so, I'll try to workaround it, otherwise we have to wait until it's fixed & released there |
Small status update: However, with this PR I was able to achieve proper "exit 0" behaviour. The following snippet will exit once everything is cleaned & closed: import org.testcontainers.DockerClientFactory;
import org.testcontainers.containers.GenericContainer;
public class Foo {
public static void main(String[] args) throws Exception {
try (GenericContainer container = new GenericContainer().withCommand("top")) {
container.start();
}
DockerClientFactory.instance().client().close();
System.out.println("QQQQ");
}
} I see some weird timeout-like behaviour after it prints "QQQQ", 30s or so, I think I know why will dig further, but at least it exits now :) |
Thanks again for the update @bsideup! I've tried the snapshot by applying the following diff to the original repro project: diff --git a/build.gradle b/build.gradle
index 4aaa259..86009a8 100644
--- a/build.gradle
+++ b/build.gradle
@@ -20,12 +20,13 @@ sourceCompatibility = 1.8
repositories {
jcenter()
+ maven { url 'https://jitpack.io' }
}
dependencies {
compile 'org.springframework.boot:spring-boot-starter-data-redis'
compile 'org.springframework.boot:spring-boot-starter-web'
- compile 'org.testcontainers:testcontainers:1.3.0'
+ compile 'com.github.testcontainers.testcontainers-java:testcontainers:-SNAPSHOT'
providedRuntime 'org.springframework.boot:spring-boot-starter-tomcat'
After running the Is it possible that the snapshot is not yet updated with your changes? |
it could be. Try |
Yeah, I saw your comment so I gave it a quite a bit more than 30 sec but it didn't help. |
@vpavic FYI there was a mistake in my previous message (wrong GAV), I fixed it, sorry! |
@vpavic D'oh! I just realized that you provided a sample project to test! Thanks a lot, I'll try it myself :) Sorry for bothering :) |
Thanks @bsideup, I tried building locally twice but the project build is hanging for me at this point:
However that might be related to me being in an environment behind a corporate network proxy ATM. |
@vpavic I got it working! diff --git a/src/main/java/sample/TestContainerBeanApplication.java b/src/main/java/sample/TestContainerBeanApplication.java
index 01a912d..0df3997 100644
--- a/src/main/java/sample/TestContainerBeanApplication.java
+++ b/src/main/java/sample/TestContainerBeanApplication.java
@@ -42,7 +42,17 @@ public class TestContainerBeanApplication extends SpringBootServletInitializer
@Bean(initMethod = "start")
public GenericContainer redisContainer() {
- return new GenericContainer("redis:3.2.9").withExposedPorts(6379);
+ return new GenericContainer("redis:3.2.9") {
+ @Override
+ public void close() {
+ super.close();
+ try {
+ dockerClient.close();
+ } catch (Exception e) {
+ e.printStackTrace();
+ }
+ }
+ }.withExposedPorts(6379);
} Output:
We need to close the client to allow JVM shutdown gracefully. Here I use Also as you can see it works with 1.3.0, no need to use the snapshot 🎉 |
That's great news, thanks for all your efforts @bsideup! So we can expect the |
@vpavic unfortunately such behaviour should not be shipped as a default one, because for most scenarios global client should stay even if container is stopped (because next test might start a new container) My workaround should work with 1.3.0/1.3.1 and very specific to your use case. We might think how to support such use case later, 1.5.0 maybe, but for now I recommend you to do it like that :) Just to give you a bit more of technical background of the issue:
What workaround does: |
Thanks for the insight and support @bsideup, we'll use the workaround you provided and keep an eye on future development on this topic. |
Thanks for digging into this and providing a fix!
Would it potentially make sense to add a flag to |
@rwinch it would not because it uses a global (read - shared) docker client, and this client should be closed after all containers are stopped. |
Is there any current workaround for forcing those netty threads to close quickly? I would use |
Any news about this issue? |
@eksd Does this workaround work for you? |
@kiview , yes, it works, but with same side effect as @samgurtman has. Netty threads close too long. |
I think I'm hitting a similar issue. I'm running my tests using Randomized Testing framework which controls that there are no leaked threads when stopping. Although I'm calling: if (container != null) {
container.close();
container = null;
} Randomized testing is complaining with:
|
@dadoonet we're waiting for a release of docker-java where this PR was merged: once it's released, we will be able to control Netty from our side and mark Netty threads as daemon threads, so that it will not prevent JVM from exiting. I'll update this issue once it's done :) |
Awesome thanks! |
In case someone had this, here is the workaround I'm using for now: @ThreadLeakFilters(filters = {TestContainerThreadFilter.class})
@ThreadLeakScope(ThreadLeakScope.Scope.SUITE)
@ThreadLeakLingering(linger = 5000) // 5 sec lingering
public abstract class AbstractITCase extends AbstractFSCrawlerTestCase {
//
} public class TestContainerThreadFilter implements ThreadFilter {
@Override
public boolean reject(Thread t) {
return t.getName().contains("dockerjava-netty") ||
t.getName().contains("pool-2-thread-1") ||
t.getName().contains("testcontainers") ||
t.getName().contains("threadDeathWatcher");
}
} |
Thanks for the update @bsideup, great news indeed! ATM I still don't see |
@vpavic oh, yes, we use Bintray to do the releases, so it's in jcenter already, but will take ~15-20 min before appearing in Maven Central, sorry for not mentioning it :) |
@bsideup I've upgraded Spring Session integration tests to TestContainers We are however still experiencing memory leak log warnings reported in spring-projects/spring-session#869. |
@vpavic good to know! The memory leak is something else and will be gone soon-ish when we replace our Netty transport with something less heavyweight (i.e. OkHttp), could you please report it separately? :) |
Thanks for the quick follow up @bsideup! Closing this one, will open a new issue for memory leak topic in a moment. |
If a container is used as a bean in Spring application (sort of like an embedded container) that runs on Tomcat, the container fails to stop when shutdown signal is sent.
Container is configured like this:
Shutdown error:
This leaves Tomcat hanging.
The sample (a simple Spring Boot app) to reproduce the problem is available in this repo. To reproduce the problem either run the integrationTest Gradle task using
./gradlew integrationTest
(this uses gretty to integration test the app on Tomcat 8), or build the WAR and run it on Tomcat (in this case the-Dspring.profiles.active=redisContainer
VM parameter is needed).To give some background - we're in the process of migrating Spring Session integration tests to TestContainers (spring-projects/spring-session#798).
JUnit
@ClassRule
based integration tests were easy to migrate however the project also contains some sample application which are packaged as WARs and then integration tested on Tomcat 8 using gretty.The idea was to provide a Spring profile which would start appropriate test container (in this case Redis) and configure the consumer (Redis connection factory) to connect to it but this has proved to be problematic due to reasons explained above.
The text was updated successfully, but these errors were encountered: