fix the issue that makes jib stuck at reading images from local docke… #4048
Conversation
| try (InputStreamReader stdout = | ||
| new InputStreamReader(inspectProcess.getInputStream(), StandardCharsets.UTF_8)) { | ||
|
|
||
| String output = CharStreams.toString(stdout); |
There was a problem hiding this comment.
I have a concern. stdout is an input stream, and at this point, you may not have reached "EOF" of the stream. That is, the process may be still running and writing characters to the stream after this line, which you will miss onward. And hypothetically, you can run into the same issue again.
There was a problem hiding this comment.
Hi. According to the documentation and source code(https://github.com/google/guava/blob/master/guava/src/com/google/common/io/CharStreams.java) of CharStreams.toString(), it reads all data from the readable. Besides, this is how other parts of this class handles outputs from the docker process.
But of course please do suggest if you have better ideas in mind. ;)
There was a problem hiding this comment.
It's a stream, and what the doc says is a different thing.
To be correct, you need to keep consuming the stream while the process is running (i.e., looping), because you never know when the process will start/resume writing data on the other side of the stream. And after you noticed the process is terminated, you need to consume the stream one last time.
However, I'd be happy just to see an improvement. So, what I suggest is to read the stream once more after process.waitFor(). It's definitely an improvement.
There was a problem hiding this comment.
lol. I do get what you mean but allow me to elaborate a bit more and you can correct me if I am wrong. :)
Reference for the behavior of InputStream.read() can be found here:
https://docs.oracle.com/javase/8/docs/api/java/io/InputStream.html#read-byte:A-
There was a problem hiding this comment.
That's good to know. One thing to correct though: you said "r is an InputStream which is not a Reader", but it is a Reader. Can you read the JDK code further to make sure it works too?
try (InputStreamReader stdout = new InputStreamReader(...) {
String output = CharStreams.toString(stdout);There was a problem hiding this comment.
And what's the rationale for using a Reader?
There was a problem hiding this comment.
Oops. My bad. It is indeed a Reader in our case. Thanks for the correction. :)
Below is the case for Reader.
As you can see, it will still exhaust the underlying Stream. Reference can be found here. https://docs.oracle.com/javase/8/docs/api/java/io/Reader.html
As for the rationale of using a Reader, I was merely following the style of existing code of the same class so that it is consistent.
There was a problem hiding this comment.
Cool then. Please just leave the comment "CharStreams.toString() will block until the end of stream is reached." That'll save people questioning the same concern as I did.
I'll let the repo maintainer to review and approve this. Ping them if necessary.
There was a problem hiding this comment.
Good idea! Added the comment as requested. :)
| try (InputStreamReader stdout = | ||
| new InputStreamReader(inspectProcess.getInputStream(), StandardCharsets.UTF_8)) { | ||
|
|
||
| String output = CharStreams.toString(stdout); |
There was a problem hiding this comment.
Cool then. Please just leave the comment "CharStreams.toString() will block until the end of stream is reached." That'll save people questioning the same concern as I did.
I'll let the repo maintainer to review and approve this. Ping them if necessary.
| } | ||
|
|
||
| return JsonTemplateMapper.readJson( | ||
| new ByteArrayInputStream(output.getBytes(StandardCharsets.UTF_8)), |
There was a problem hiding this comment.
Thanks for the tips. Updated the code to use the overload version you suggested.
|
But any news? actually We are working with a local version |
|
|
||
| if (inspectProcess.waitFor() != 0) { | ||
| throw new IOException( | ||
| "'docker inspect' command failed with error: " + getStderrOutput(inspectProcess)); |
There was a problem hiding this comment.
@EternalWind I think it would be safer to also exhaust StdErr stream the same way like StdOut to avoid the similar "being stuck" issue because of StdErr being full?
StdErr stream may have some data regardless whether process exited with success or error, so it should be read regardless of waitFor() result.
Fixes #4046 🛠️
The exact issue description, root cause analysis and proposed fix plan are mentioned in the issue.