Skip to content

Fixes #1448 #1449

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.

Sign up for GitHub

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

Closed
tejksat wants to merge 2 commits into from
Closed

Fixes #1448 #1449

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c192978
Reformat AttachContainerCmdIT.java according to EditorConfig settings
tejksat Aug 11, 2020
2ca7dd3
Shutdown stdin after closing the corresponding stream on docker-java …
tejksat Aug 12, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions ...ava-transport-okhttp/src/main/java/com/github/dockerjava/okhttp/HijackingInterceptor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ public Response intercept(Chain chain) throws IOException {
while (sink.isOpen()) {
int aByte = stdin.read();
if (aByte < 0) {
sink.close();
break;
}
sink.writeByte(aByte);
Expand Down
10 changes: 8 additions & 2 deletions ...er-java-transport-okhttp/src/main/java/com/github/dockerjava/okhttp/UnixDomainSocket.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,8 @@ public static native int recv(int fd, byte[] buffer, int count, int flags)
public static native int send(int fd, byte[] buffer, int count, int flags)
throws LastErrorException;

public static native int shutdown(int fd, boolean read, boolean write) throws LastErrorException;

public static native int close(int fd) throws LastErrorException;

public static native String strerror(int errno);
Expand Down Expand Up @@ -196,8 +198,12 @@ public void shutdownInput() {
// do nothing
}

public void shutdownOutput() {
// do nothing
public void shutdownOutput() throws IOException {
try {
shutdown(fd, true, false);
} catch (LastErrorException lee) {
throw new IOException("native shutdown() failed : " + formatError(lee));
}
}

public static class SockAddr extends Structure {
Expand Down
184 changes: 128 additions & 56 deletions docker-java/src/test/java/com/github/dockerjava/cmd/AttachContainerCmdIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import com.github.dockerjava.api.command.InspectContainerResponse;
import com.github.dockerjava.api.model.Frame;
import com.github.dockerjava.api.model.StreamType;
import com.github.dockerjava.utils.LogContainerTestCallback;
import org.junit.Assume;
import org.junit.Rule;
import org.junit.Test;
Expand All @@ -18,6 +19,7 @@
import java.io.InputStream;
import java.io.PipedInputStream;
import java.io.PipedOutputStream;
import java.io.PrintStream;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.TimeUnit;

Expand All @@ -26,8 +28,8 @@
import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;
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;
Expand All @@ -51,10 +53,10 @@ public void attachContainerWithStdin() throws Exception {
String snippet = "hello world";

CreateContainerResponse container = dockerClient.createContainerCmd("busybox")
.withCmd("/bin/sh", "-c", "sleep 1 && read line && echo $line")
.withTty(false)
.withStdinOpen(true)
.exec();
.withCmd("/bin/sh", "-c", "sleep 1 && read line && echo $line")
.withTty(false)
.withStdinOpen(true)
.exec();

LOG.info("Created container: {}", container.toString());
assertThat(container.getId(), not(is(emptyString())));
Expand Down Expand Up @@ -101,9 +103,9 @@ public void attachContainerWithoutTTY() throws Exception {
String snippet = "hello world";

CreateContainerResponse container = dockerClient.createContainerCmd(DEFAULT_IMAGE)
.withCmd("echo", snippet)
.withTty(false)
.exec();
.withCmd("echo", snippet)
.withTty(false)
.exec();

LOG.info("Created container: {}", container.toString());
assertThat(container.getId(), not(is(emptyString())));
Expand All @@ -115,16 +117,16 @@ public void attachContainerWithoutTTY() throws Exception {
public void onNext(Frame frame) {
assertThat(frame.getStreamType(), equalTo(StreamType.STDOUT));
super.onNext(frame);
};
}
};

dockerClient.attachContainerCmd(container.getId())
.withStdErr(true)
.withStdOut(true)
.withFollowStream(true)
.withLogs(true)
.exec(callback)
.awaitCompletion(30, TimeUnit.SECONDS);
.withStdErr(true)
.withStdOut(true)
.withFollowStream(true)
.withLogs(true)
.exec(callback)
.awaitCompletion(30, TimeUnit.SECONDS);
callback.close();

assertThat(callback.toString(), containsString(snippet));
Expand All @@ -135,7 +137,7 @@ public void attachContainerWithTTY() throws Exception {
DockerClient dockerClient = dockerRule.getClient();

File baseDir = new File(Thread.currentThread().getContextClassLoader()
.getResource("attachContainerTestDockerfile").getFile());
.getResource("attachContainerTestDockerfile").getFile());

String imageId = dockerRule.buildImage(baseDir);

Expand All @@ -151,15 +153,15 @@ public void attachContainerWithTTY() throws Exception {
public void onNext(Frame frame) {
assertThat(frame.getStreamType(), equalTo(StreamType.RAW));
super.onNext(frame);
};
}
};

dockerClient.attachContainerCmd(container.getId())
.withStdErr(true)
.withStdOut(true)
.withFollowStream(true)
.exec(callback)
.awaitCompletion(15, TimeUnit.SECONDS);
.withStdErr(true)
.withStdOut(true)
.withFollowStream(true)
.exec(callback)
.awaitCompletion(15, TimeUnit.SECONDS);
callback.close();

LOG.debug("log: {}", callback.toString());
Expand All @@ -178,9 +180,9 @@ public void attachContainerStdinUnsupported() throws Exception {
String snippet = "hello world";

CreateContainerResponse container = dockerClient.createContainerCmd(DEFAULT_IMAGE)
.withCmd("echo", snippet)
.withTty(false)
.exec();
.withCmd("echo", snippet)
.withTty(false)
.exec();

LOG.info("Created container: {}", container.toString());
assertThat(container.getId(), not(is(emptyString())));
Expand All @@ -192,19 +194,19 @@ public void attachContainerStdinUnsupported() throws Exception {
public void onNext(Frame frame) {
assertThat(frame.getStreamType(), equalTo(StreamType.STDOUT));
super.onNext(frame);
};
}
};

InputStream stdin = new ByteArrayInputStream("".getBytes());

dockerClient.attachContainerCmd(container.getId())
.withStdErr(true)
.withStdOut(true)
.withFollowStream(true)
.withLogs(true)
.withStdIn(stdin)
.exec(callback)
.awaitCompletion(30, TimeUnit.SECONDS);
.withStdErr(true)
.withStdOut(true)
.withFollowStream(true)
.withLogs(true)
.withStdIn(stdin)
.exec(callback)
.awaitCompletion(30, TimeUnit.SECONDS);
callback.close();
}

Expand All @@ -217,33 +219,33 @@ public void attachContainerClosesStdoutWhenContainerExits() throws Exception {
DockerClient dockerClient = dockerRule.getClient();

CreateContainerResponse container = dockerClient.createContainerCmd(DEFAULT_IMAGE)
.withCmd("echo", "hello")
.withTty(false)
.exec();
.withCmd("echo", "hello")
.withTty(false)
.exec();
LOG.info("Created container: {}", container.toString());

CountDownLatch gotLine = new CountDownLatch(1);
try (
ResultCallback.Adapter<Frame> resultCallback = dockerClient.attachContainerCmd(container.getId())
.withStdOut(true)
.withStdErr(true)
.withFollowStream(true)
.exec(new ResultCallback.Adapter<Frame>() {
@Override
public void onNext(Frame item) {
LOG.info("Got frame: {}", item);
if (item.getStreamType() == StreamType.STDOUT) {
gotLine.countDown();
}
super.onNext(item);
}

@Override
public void onComplete() {
LOG.info("On complete");
super.onComplete();
}
})
ResultCallback.Adapter<Frame> resultCallback = dockerClient.attachContainerCmd(container.getId())
.withStdOut(true)
.withStdErr(true)
.withFollowStream(true)
.exec(new ResultCallback.Adapter<Frame>() {
@Override
public void onNext(Frame item) {
LOG.info("Got frame: {}", item);
if (item.getStreamType() == StreamType.STDOUT) {
gotLine.countDown();
}
super.onNext(item);
}

@Override
public void onComplete() {
LOG.info("On complete");
super.onComplete();
}
})
) {
resultCallback.awaitStarted(5, SECONDS);
LOG.info("Attach started");
Expand All @@ -257,6 +259,76 @@ public void onComplete() {
}
}

/**
* The process in Docker container should receive EOF when reading its
* stdin after the corresponding stream is closed on docker-java side in
* case when the container has been created with StdinOnce flag set to
* true.
*/
@Test
public void closeStdinWithStdinOnce() throws Exception {
Assume.assumeTrue("supports stdin attach", getFactoryType().supportsStdinAttach());
Assume.assumeFalse("not fixed for Apache HttpClient 5.0", getFactoryType().equals(FactoryType.HTTPCLIENT5));
Copy link
Copy Markdown
Member

@bsideup bsideup Aug 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it a WIP or you was unable to fix it for AHC5?

Copy link
Copy Markdown
Contributor Author

@tejksat tejksat Aug 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I addressed this problem because my colleague at JetBrains stuck into the situation where our Python helper script misbehaved. And we use only OkHttp implementation.

However I dived into AHC5 implementation. It has pretty similar UnixDomainSocket class. But fixing only this class does not help because org.apache.hc.core5.http.impl.io.ContentLengthOutputStream.close() does not call close() method on underlying outputStream (which belongs to UnixDomainSocket that could be fixed) and, unfortunately,ContentLengthOutputStream class is situated in org.apache.httpcomponents.core5:httpcore5:5.0 artifact.

Here is the stacktrace where magic does not happen:

"docker-java-httpclient5-hijacking-stream-1739943297@6989" daemon prio=5 tid=0x3d nid=NA runnable
  java.lang.Thread.State: RUNNABLE
	  at org.apache.hc.core5.http.impl.io.ContentLengthOutputStream.close(ContentLengthOutputStream.java:92)
	  at org.apache.hc.core5.http.impl.io.DefaultBHttpClientConnection.sendRequestEntity(DefaultBHttpClientConnection.java:154)
	  at com.github.dockerjava.httpclient5.HijackingHttpRequestExecutor.lambda$executeHijacked$0(HijackingHttpRequestExecutor.java:82)
	  at com.github.dockerjava.httpclient5.HijackingHttpRequestExecutor$$Lambda$233.1769119455.run(Unknown Source:-1)
	  at java.lang.Thread.run(Thread.java:834)

Copy link
Copy Markdown

@mrtamm mrtamm Oct 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have investigated the problem in AHC5 implementation. Although the org.apache.hc.core5.http.impl.io.ContentLengthOutputStream.close() method does not close the underlying OutputStream, it is possible to shut down the OutputStream in com.github.dockerjava.httpclient5.HijackingHttpRequestExecutor class right after conn.sendRequestEntity(fakeRequest); call:

if (conn instanceof ManagedHttpClientConnection) {
    ((ManagedHttpClientConnection) conn).getSocket().shutdownOutput();
}

However, for that to work, similar changes to the UnixDomainSocket class (under docker-java-transport-httpclient5) are needed because, by default, UnixDomainSocket.shutdownOutput() does nothing.

Copy link
Copy Markdown
Member

@bsideup bsideup Oct 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mrtamm thanks for investigating, this is very helpful!

@tejksat will you be able to apply the changes as per @mrtamm's advice?

Copy link
Copy Markdown
Member

@bsideup bsideup Feb 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@tejksat gentle ping :)


DockerClient dockerClient = dockerRule.getClient();

File baseDir = new File(Thread.currentThread().getContextClassLoader()
.getResource("closeStdinWithStdinOnce").getFile());

String imageId = dockerRule.buildImage(baseDir);

CreateContainerResponse container = dockerClient.createContainerCmd(imageId)
.withStdinOpen(true)
.withStdInOnce(true)
.exec();
LOG.info("Created container: {}", container.toString());

String lastLineToStdout = "The last line to stdout";
CountDownLatch gotLastLineMarker = new CountDownLatch(1);
PipedOutputStream stdinSource = new PipedOutputStream();
PipedInputStream stdin = new PipedInputStream(stdinSource);

try (
ResultCallback.Adapter<Frame> resultCallback = dockerClient.attachContainerCmd(container.getId())
.withStdOut(true)
.withStdErr(true)
.withStdIn(stdin)
.withFollowStream(true)
.exec(new LogContainerTestCallback() {
@Override
public void onNext(Frame frame) {
super.onNext(frame);
if (log.toString().contains(lastLineToStdout)) {
gotLastLineMarker.countDown();
}
}
})
) {
resultCallback.awaitStarted(5, SECONDS);
LOG.info("Attach started");

dockerClient.startContainerCmd(container.getId()).exec();
LOG.info("Container started");

new Thread(() -> {
try (PrintStream printStream = new PrintStream(stdinSource, true)) {
// pass the marker line to let the script echo it after stdin is closed
printStream.println(lastLineToStdout);
// pass several random lines to container's stdin
for (int i = 1; i < 10; i++) {
printStream.println(i + " line");
}
}
// note that at this point stdin pipe stream to Docker container is closed
}).start();

assertTrue("Stdout should still function after closing stdin", gotLastLineMarker.await(15, SECONDS));

assertTrue("The script should finish quickly after stdin is closed on docker-java side",
resultCallback.awaitCompletion(15, SECONDS));
}
}

public static class AttachContainerTestCallback extends ResultCallback.Adapter<Frame> {
private StringBuffer log = new StringBuffer();

Expand Down
9 changes: 9 additions & 0 deletions docker-java/src/test/resources/closeStdinWithStdinOnce/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
FROM busybox:latest

ADD echo_stdin.sh /tmp/

RUN mkdir -p /usr/local/bin
RUN cp /tmp/echo_stdin.sh /usr/local/bin/ && chmod +x /usr/local/bin/echo_stdin.sh

CMD ["echo_stdin.sh"]

7 changes: 7 additions & 0 deletions docker-java/src/test/resources/closeStdinWithStdinOnce/echo_stdin.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
#!/bin/sh
# Read the marker line to echo it after stdin is closed
read -r marker_line_after_eof_from_stdin
# Echo all lines received from stdin
while read -r line; do echo "$line"; done
# Show that stdout still function after closing stdin
echo "$marker_line_after_eof_from_stdin"