Fix #1492 by using awaitStarted() to preserve the order if invocations (#1494)

tejksat · web-flow · commit 0d08a33dbbd3 · 2020-11-16T15:31:45.000+01:00
The problem is that using `awaitStarted()` on `attach`'s callback on a non-running container leads to the infinite wait for Jersey transport implementation.

This happens when `SelectiveLoggingFilter` is used in `DefaultInvocationBuilder` where it is created with `printEntity = true` parameter. In this case the implementation of `ClientResponseFilter.filter(...)` method in the base `LoggingFilter` class blocks at `responseContext.hasEntity()` until the data is available in the response. This happens because `hasEntity()` reads a single byte to check whether there is any data available in the stream. In the case of `attach` command this method blocks until there is data in stdout/stderr streams. Having data from the streams requires container running, which we cannot do safely without `awaitStarted()` because `attach` command is performed in a separate thread and we need to run `start` only _after_ `attach` request is executed not to loose the data from stdout/stderr streams.
diff --git a/docker-java-transport-jersey/src/main/java/com/github/dockerjava/jaxrs/JerseyDockerHttpClient.java b/docker-java-transport-jersey/src/main/java/com/github/dockerjava/jaxrs/JerseyDockerHttpClient.java
@@ -165,7 +165,7 @@ private JerseyDockerHttpClient(
         RequestConfig.Builder requestConfigBuilder = RequestConfig.custom();
 
         // logging may disabled via log level
-        clientConfig.register(new SelectiveLoggingFilter(LOGGER, true));
+        clientConfig.register(new SelectiveLoggingFilter(LOGGER, false));
 
         if (readTimeout != null) {
             requestConfigBuilder.setSocketTimeout(readTimeout);
diff --git a/docker-java/src/test/java/com/github/dockerjava/cmd/AttachContainerCmdIT.java b/docker-java/src/test/java/com/github/dockerjava/cmd/AttachContainerCmdIT.java
@@ -28,8 +28,7 @@
 import static org.hamcrest.Matchers.emptyString;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.not;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.*;
 
 /**
  * @author Kanstantsin Shautsou
@@ -80,6 +79,9 @@ public void onNext(Frame frame) {
                 .withStdIn(in)
                 .exec(callback);
 
+            assertTrue("Processing of the response should start shortly after executing `attachContainerCmd`",
+                callback.awaitStarted(5, SECONDS));
+
             dockerClient.startContainerCmd(container.getId()).exec();
 
             out.write((snippet + "\n").getBytes());
@@ -123,6 +125,9 @@ public void onNext(Frame frame) {
             .withLogs(true)
             .exec(callback);
 
+        assertTrue("Processing of the response should start shortly after executing `attachContainerCmd`",
+            callback.awaitStarted(5, SECONDS));
+
         dockerClient.startContainerCmd(container.getId()).exec();
 
         callback.awaitCompletion(30, TimeUnit.SECONDS);
@@ -164,6 +169,9 @@ public void onNext(Frame frame) {
             .withFollowStream(true)
             .exec(callback);
 
+        assertTrue("Processing of the response should start shortly after executing `attachContainerCmd`",
+            callback.awaitStarted(5, SECONDS));
+
         dockerClient.startContainerCmd(container.getId()).exec();
 
         callback.awaitCompletion(15, TimeUnit.SECONDS);
@@ -213,6 +221,9 @@ public void onNext(Frame frame) {
             .withStdIn(stdin)
             .exec(callback);
 
+        assertFalse("Processing of the response is not expected to be started" +
+            " because `attachContainerCmd` with stdin is not supported", callback.awaitStarted(5, SECONDS));
+
         dockerClient.startContainerCmd(container.getId()).exec();
 
         callback.awaitCompletion(30, TimeUnit.SECONDS);
