Skip to content
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.

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

Support writable streams (process STDIN) #33

Open
clue opened this issue Apr 25, 2016 · 6 comments · May be fixed by #63
Open

Support writable streams (process STDIN) #33

clue opened this issue Apr 25, 2016 · 6 comments · May be fixed by #63

Comments

@clue
Copy link
Owner

clue commented Apr 25, 2016

Quite a few of the API endpoints take advantage of readable streaming APIs.

In the future, we should also look into streaming outgoing data even after having received the HTTP response header. This is relevant for attaching to read/write streams, such as writing to the STDIN of attached streams.

@clue
Copy link
Owner Author

clue commented Apr 27, 2016

This also refs the command streaming APIs in #13 and #14 (but not #12), see also #36.

@clue clue removed this from the v0.4.0 milestone Sep 18, 2019
@clue
Copy link
Owner Author

clue commented Sep 22, 2019

Implementation depends on clue/reactphp-buzz#135. That issue is nontrivial giving the current APIs, but otherwise this should be fairly straight-forward in this library once the underlying support is there.

@clue
Copy link
Owner Author

clue commented Jul 12, 2020

Implementation now depends on reactphp/http#376.

@bosunski
Copy link

@clue how can I help on this? I personally will like to see a writable stream available for execStartStream.

@clue
Copy link
Owner Author

clue commented Jul 25, 2020

@bosunski This currently depends on reactphp/http#376 first in order to support the Upgrade: tcp request header used for https://docs.docker.com/engine/api/v1.24/#42-hijacking. Once that is resolved, implementing this here shouldn't be a lot of work.

We've recently made some major changes to the underlying HTTP client and are currently working on some major new features. This will also allow us to expose the required APIs for this feature, so this really only a matter of time at the moment.

PRs to help with this are always very much appreciated! 👍

@bosunski bosunski linked a pull request Jul 27, 2020 that will close this issue
@clue
Copy link
Owner Author

clue commented Aug 12, 2020

I've just toyed around with this some more an came up with a working prototype! 🎉

It turns out the Upgrade: tcp request header is entirely optional, so this features doesn't necessarily depend on reactphp/http#376. If we don't use this request header, we can pass in the STDIN as a request body. Here's the gist as a bash script:

$ docker run -id --rm --name foo busybox sh
$ (echo -en "POST /containers/foo/attach?stream=1&stdin=1&stdout=1&stderr=1 HTTP/1.1\r\nHost: localhost\r\n\r\n";sleep 0.1;echo -en "whoami\n"; sleep 0.1) | nc -N -U /var/run/docker.sock && echo

I've also created a working prototype for this with ReactPHP. It boils down to using the existing containerAttachStream() API, but passing a ThroughStream for the request body and then combining this with the request stream in a CompositeStream. Known limitations include requiring a (pointless) Content-Length request header to avoid the Transfer-Encoding: chunked request header at the moment and writes to this resulting DuplexStreamInterface are currently discarded before the response headers have been received (see sleep 0.1 in above example).

I don't currently have an immediate use case for this feature, so I'll have to postpone this for now. As much as I'd like to commit to specific date, I can only say that we will get to this feature as soon as time permits.

If you need this for a commercial project and you want to help sponsor this feature, feel free to reach out and I'm happy to take a look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants