#369 Daemonize threads #646
Conversation
…s and group them
| compile 'org.rnorth.visible-assertions:visible-assertions:2.1.0' | ||
|
|
||
| shaded ('com.github.docker-java:docker-java:3.0.12') { | ||
| shaded ('com.github.docker-java:docker-java:3.1.0-rc-2') { |
There was a problem hiding this comment.
it's fine to use rc-2 here because we're shading it anyway. The changes from 3.0.x doesn't seem to break anything
There was a problem hiding this comment.
Seems OK, but let's migrate to a non-rc version as soon as we can.
| @@ -0,0 +1,304 @@ | |||
| package org.testcontainers.dockerclient.transport; | |||
There was a problem hiding this comment.
check the javadoc for this class. This is almost one-to-one version from docker-java with some modifications.
The whole class to be replaced with another transport (like OkHttp), so I wouldn't bother refactoring it any futher
There was a problem hiding this comment.
👍 I'll not review this in too much detail as it's largely based on well-tested principles.
|
|
||
| @Test | ||
| public void testThatAllThreadsAreDaemons() throws Exception { | ||
| ProcessBuilder processBuilder = new ProcessBuilder( |
There was a problem hiding this comment.
we need to fork the JVM, otherwise we might already initialize the threads and the result will not reflect the scenario ("should not create non-daemon threads")
| DaemonTest.class.getCanonicalName() | ||
| ); | ||
|
|
||
| assert processBuilder.inheritIO().start().waitFor() == 0; |
There was a problem hiding this comment.
It's not critical, but why not an assertEquals call here?
|
|
||
| @Test | ||
| public void testThatAllThreadsAreDaemons() throws Exception { | ||
| ProcessBuilder processBuilder = new ProcessBuilder( |
| import java.util.Set; | ||
| import java.util.stream.Collectors; | ||
|
|
||
| public class DaemonTest { |
There was a problem hiding this comment.
Maybe worth a comment here to explain why there's a main method, roughly what the test does?
People might jump to conclusions, perhaps!
| * Changes: | ||
| * - daemonized thread factory | ||
| * - thread group | ||
| * - the logging handler removed |
There was a problem hiding this comment.
Does this mean no more log noise from netty?
If so, 😻 x 💯!
Also, it'd be worthy of a mention in the changelog.
There was a problem hiding this comment.
:D Yes, it does :)
It of course makes logs less debuggable but since not every TC user knows how to configure the logger IMO this is a much better UX
| @@ -0,0 +1,304 @@ | |||
| package org.testcontainers.dockerclient.transport; | |||
There was a problem hiding this comment.
👍 I'll not review this in too much detail as it's largely based on well-tested principles.
| compile 'org.rnorth.visible-assertions:visible-assertions:2.1.0' | ||
|
|
||
| shaded ('com.github.docker-java:docker-java:3.0.12') { | ||
| shaded ('com.github.docker-java:docker-java:3.1.0-rc-2') { |
There was a problem hiding this comment.
Seems OK, but let's migrate to a non-rc version as soon as we can.
No description provided.