HttpDuplex.end() breaks output

[jonasfj]

So playing with attach streams... specifically for docker exec, I've found that you can't end the stdin stream... The HttpDuplex.end() implementation breaks the TCP stream - not just closing this side.

So if you do the equivalent of echo "test" | docker exec CONTAINER wc -c you don't get the expected output.

Looking at the docker implementation, it seems that we should:

1. Set Connection: upgrade and Upgrade: tcp headers, see docker cli code.
2. And then just hijack the raw TCP socket and forget all about HTTP, or ensure that HttpDuplex only closes this side of the request (should be the same right?).

Either way, this seems to fix my issue:

HttpDuplex.prototype.end = function(chunk, encoding, cb) {
  var req = this.req;
  return req.end(chunk, encoding, function() {
    req.socket.end();
    if (cb) {
      cb();
    }
  });
};

Not sure why I'm having to close the TCP socket. It ought to just end on it's own.
Current code does this._output.socket.destroy(); which is probably why I miss any output arriving after stdin is closed.

[jonasfj]

Also if I do:

exec.start({stdin: true, stream: true, ...}, function(err, stream) {
  stream = stream.req.socket;
  // Attach as normally...
});

Then everything works as expected... So perhaps we don't need the HttpDuplex wrapper at all.
stream.req.socket.end() closes the stdin side of the socket as expected.

[jonasfj]

Update... I've just realized that I loose some initial output if I use stream = stream.req.socket.
So this probably have to the the upgrade thing.

[jonasfj]

@EggyLv999, any chance you can take a look at this.
I'm not sure if docker-modem is easily fixed, or we need some special methods to support docker exec.

[JohnyDays]

Also having this issue, being unable to manually end the connection

[JohnyDays]

Tried your fix @jonasfj, worked for me. Making a fork for now

[jonasfj]

@JohnyDays, I experienced other issues with my fix. I suspect the best solution is to send the Upgrade: tcp headers as expected by docker.

[JohnyDays]

I was using the logs() function to monitor logging and look for a signal that a certain container is ready for action. But it just wasn't working, there was no reliable way to close the connection that did not result in disaster. So I am now polling the logs every X seconds (with a slight modification in a fork, not to use streams) and checking that way

[dcolens]

FYI https://github.com/mafintosh/docker-remote-api does not have this issue.

[apocas]

This issue is a bit (a lot) old, lost track of it, since then (long time ago) we have implemented hijacking.

Described in here: https://github.com/apocas/dockerode#streams-goodness

Do you have this issue using hijacking?

[dcolens]

Nope I did not try, the test I wrote back in 2015 showed the issue: apocas/dockerode#136 <apocas/dockerode#136> the way it was re-written was not right because it did not use the stream to end the connection but the exit command. Did

…

On 10 Mar 2017, at 23:00, Pedro Dias ***@***.***> wrote: This issue is a bit old, lost track of it, since then we have implemented hijacking. Described in here: https://github.com/apocas/dockerode#streams-goodness <https://github.com/apocas/dockerode#streams-goodness> Do you have this issue using hijacking? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#48 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABJCEpEyHv7FgvG8rVfm1BLvE2Z5oMr8ks5rkcfxgaJpZM4FoQ0r>.