Fix incomplete read in the DockerMultiplexedInputStream

[a-ba]

Fix #628 (which should be reopened) and https://issues.jenkins-ci.org/browse/JENKINS-53669

readInternal() assumed that the 8-byte header of a docker stream will always be fully read in one call. This is just misleaded.

The assumption would be okay if the network always had a high bandwidth and low jitter, but real-world cases we may random delays (because of packet losses, buffering, ...) that causes random read errors, eg:

FATAL: java.io.IOException: Expected 8 bytes application/vnd.docker.raw-stream header, got 2

Note: the PR also fixes a missing exit case when reaching EOF in the stderr channel

[pjdarton]

The initial changes look good; it looks like a perfect fit to solving #628

The handling of stderr, however, leaves a lot to be desired (mainly due to the original code). I agree that the EOF handling is an improvement, but I think that now we've seen it, it would be wrong to leave it at that and we really ought to "fix it properly".

Ideally, we should also have some unit tests for this code too (if we'd had some to start with, #628 probably would've been found by the tests...)

[pjdarton]

@ndeloof It looks like you were the original author of this code... Can you confirm that I've understood it correctly?

• I believe that what it's doing is reading a stream that's of the format <1-byte-type><3 bytes ignored><4-byte-length> that then can repeat.
• I believe that it's intended to return the "payload" only where the 1-byte-type is 1 (stdout).
• I believe that the stream can also contain payload where the 1-byte-type is 2 (stderr) and the intent was to record that "not expected" data in a way that allows the Jenkins admin to see it.

So, the existing code's handling of the stderr case (type==2)...

• should really log to a Logger instead of Jenkins' stderr,
• should handle an EOF by outputting the incomplete stderr message.

...and would you also agree that an effective unit-test could be to take some example input data paired with expected output, and then check that the code handled it correctly both when it was provided by the multiplexed stream "all at once" and when it's only available one byte at a time.
Or can you think of an easier way of unit-testing it?

[a-ba]

@ndeloof It looks like you were the original author of this code... Can you confirm that I've understood it correctly?

* I believe that what it's doing is reading a stream that's of the format <1-byte-type><3 bytes ignored><4-byte-length> that then can repeat.

* I believe that it's intended to return the "payload" only where the 1-byte-type is 1 (stdout).

* I believe that the stream can also contain payload where the 1-byte-type is 2 (stderr) and the intent was to record that "not expected" data in a way that allows the Jenkins admin to see it.

this is the docker 'stream' format, it is described in the API at:
https://docs.docker.com/engine/api/v1.32/#operation/ContainerAttach

[jakub-bochenski]

I've been using 1.1.6-rc867.6e6f38dd6134 for 4 days now and didn't get a single "Expected 8 bytes ..." error

[a-ba]

I applied the suggested changes + wrote tests for this class

[pjdarton]

@a-ba Thanks for persevering with this; it's much appreciated.