Skip to content

Support interactive stdin/stdout streams#136

Merged
softprops merged 3 commits intosoftprops:masterfrom
dylanmckay:support-stream-tcp-upgrade--upstream
Dec 22, 2018
Merged

Support interactive stdin/stdout streams#136
softprops merged 3 commits intosoftprops:masterfrom
dylanmckay:support-stream-tcp-upgrade--upstream

Conversation

@dylanmckay
Copy link
Copy Markdown
Contributor

@dylanmckay dylanmckay commented Dec 7, 2018

This adds support for streaming stdin, stderr, and stdout independently
to a running container.

The underlying API is futures-based, meaning the code is implemented
asynchronously. A synchronous API is also exposed, which is implemented
by simply waiting on the asynchronous API futures.

This also modifies the existing Tty logic so that the storage type of
the data is a Vec rather than a String. This is also how the Rust
standard library persists data from the standard streams. In my
particular application, I'm using stdin/stdout as the communication
method between a container a host application. In it, a byte-based protocol is
used.

Streaming works by performing a TCP upgrade; upgrading a higher-level
HTTP connection to a lower-level TCP byte stream upon agreement with the
server. Docker will automatically upgrade HTTP container log requests to
TCP byte streams of a custom std{in,out,err} multiplexing protocol if
the client requests it with the 'Connection: Upgrade' header.

How did you verify your change:

I've got a project that implements a nontrivial byte-based protocol to send command/reply frames from the host app to the container over stdout/stdin. I can successfully parse, send, and process frames in both directions asynchronously with this patch.

What (if anything) would need to be called out in the CHANGELOG for the next release:

  • The Container::logs function now returns a tty::Chunk type containing raw bytes, rather than a TtyLine object containing a UTF-8 string.
  • The standard streams of container logs can now be independently managed and communicated with

This adds support for streaming stdin, stderr, and stdout independently
to a running container.

The underlying API is futures-based, meaning the code is implemented
asynchronously. A synchronous API is also exposed, which is implemented
by simply waiting on the asynchronous API futures.

This also modifies the existing Tty logic so that the storage type of
the data is a Vec<u8> rather than a String. This is also how the Rust
standard library persists data from the standard streams. In my
particular application, I'm using stdin/stdout as the communication
method between a container a host application. In it, a byte-based protocol is
used.

Streaming works by performing a TCP upgrade; upgrading a higher-level
HTTP connection to a lower-level TCP byte stream upon agreement with the
server. Docker will automatically upgrade HTTP container log requests to
TCP byte streams of a custom std{in,out,err} multiplexing protocol if
the client requests it with the 'Connection: Upgrade' header.
Comment thread src/transport.rs Outdated

self.send_request(req).and_then(|res| {
if res.status() != StatusCode::SWITCHING_PROTOCOLS {
panic!("Our server didn't upgrade: {}", res.status());
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It might be better to return a failed Future here, instead of panicking?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good idea

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@dylanmckay
Copy link
Copy Markdown
Contributor Author

I've also added Chunk::as_string, Chunk::as_string_lossy accessors, and updated the examples to use them.

Tests pass on my machine, I can compile all examples, and all tests pass.

One of the existing PRs to fix the existing rustfmt error in shiplift/master need to be merged before CI will pass for this change. I've included it in one of my smaller PRs, #134, but it looks like #137 also does it.

@softprops
Copy link
Copy Markdown
Owner

apologize for the slow feedback cycle. I've been on vacation. there are some changes I think that need to happen in my travis build. I think pr check errors are unrelated to your changes

@softprops softprops merged commit 6b5f0c0 into softprops:master Dec 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants