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

Async api #128

Merged
merged 31 commits into from Nov 14, 2018

Conversation

Projects
None yet
3 participants
@abusch
Contributor

abusch commented Oct 16, 2018

What did you implement:

Migrate the whole API to an asynchronous style i.e. every method returns either a Future or a Stream which will need to be executed in a Tokio runtime by the user. This remove any use of block_on() internally, as well as the need to hold a Runtime inside Transport. This allows proper streaming (e.g. for stats or events), and integration with other async crates.

Closes: #111

How did you verify your change:

I haven't really yet, except by running some of the examples :)

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

This is a major API change that affects pretty much every method.

Things that still need to be done:

  • Finish implementing the methods I've commented out (see TODO(abusch) in the code)
  • Finish porting the remaining examples
  • Introduce a DockerFuture<T> type (and DockerStream<T>?) to simplify method signature (maybe?)
  • Fix tests and doc tests
  • Clean-up imports / clippy warnings

abusch added some commits Oct 10, 2018

Refactored Transport for better async use
Still a bit rough, but it now builds a big future using combinators. It
still does one `Runtime::block_on()` to keep the existing API, but this
is a first up before making the whole API async.
Migrate most APIs to be Future-based
I still need to finish a few of the more tricky ones that I've commented
out for now, but most of it compiles and some examples work. In
particular, `Docker::stats()` now properly returns an async stream of
stats.
use shiplift::Docker;
use tokio::prelude::*;

This comment has been minimized.

@softprops

softprops Oct 17, 2018

Owner

When possible I try to avoid wildcards imports because the hide potentially useful documentation. Since these examples files serve as documentation I'd like it to be a bit more clear. What prelude items does this file use/need?

This comment has been minimized.

@abusch

abusch Oct 17, 2018

Contributor

Fair enough. I thought this was the recommended way to use Tokio.

src/lib.rs Outdated
&self,
opts: &EventsOptions,
) -> Result<Vec<Event>> {
pub fn events(&self, opts: &EventsOptions) -> impl Stream<Item = Event, Error = Error> {

This comment has been minimized.

@softprops

softprops Oct 17, 2018

Owner

😍 good choice. streams are an awesome choice. Doesn't have to happen here but I just noticed the pre-existing typo in this rustdoc: interator -> iterator. it was both incorrect and a typo. we may just want to say something like Returns a stream of docker events. The docs for this crate could use some polish but that can definately be a followup

self.stream(method, endpoint, body)
.concat2()
.and_then(|v| String::from_utf8(v).map_err(Error::Encoding).into_future())
// debug!("{} raw response: {}", endpoint, body);

This comment has been minimized.

@softprops

softprops Oct 17, 2018

Owner

What do you think about keeping the debug! log? I've often found these useful in cases where I need to reproduce a deserialization problem with a type mapping. The errors that bubble up from serde sometimes do not contain enough contextual information to give you an idea of what is in error.

| StatusCode::CREATED
| StatusCode::SWITCHING_PROTOCOLS
| StatusCode::NO_CONTENT => future::ok(res),
_ => future::err(Error::Fault {

This comment has been minimized.

@softprops

softprops Oct 17, 2018

Owner

much much much nicer!

// Extract the error message content from an HTTP response that
// contains a Docker JSON error structure.
// fn get_error_message(&self, body: &str) -> Option<String> {
// match String::from_utf8(chunk.into_iter().collect()) {

This comment has been minimized.

@softprops

softprops Oct 17, 2018

Owner

now that we're using serde, if there's a standard way to represent it with a struct. we have more civilized tools now!

This comment has been minimized.

@abusch

abusch Oct 17, 2018

Contributor

Yeah, I'm struggling a bit as to how best to reintroduce this method, because in the error case we need to read the whole body and parse it to get the error message, but in the success page we just want to return a stream of body chunks... Haven't find the right sequence of combinators yet :)

@softprops

This comment has been minimized.

Owner

softprops commented Oct 17, 2018

@abusch I just merged a pull for exposing ports on the container builder which caused a conflict with this branch. Just wanted to give you a heads up. I took a look over this branch. It looks awesome! Thanks for taking all of this on.

abusch added some commits Oct 17, 2018

Add adapter from Stream of Chunks to AsyncRead
Having an `AsyncRead` is required to be able to use the `FramedRead` and
`Decoder` stuff from tokio_codec. This code is "borrowed" from
https:/github.com/ferristseng/rust-ipfs-api though should probably be
moved to its own crate or to tokio_codec.
Fix Container::logs()
It now properly demuxes stdout/stderr, and returns a `Stream<Item =
TtyLine>`.
Use LineCodec for streaming JSON
Although in my limited testing it seemed to work fine, there is no
guarantee that 1 chunk == 1 piece of valid JSON. However, each JSON
structure seems to be serialized on one line, so use LineCodec to turn
the body into a stream of lines, then deserialize over this.
@abusch

This comment has been minimized.

Contributor

abusch commented Oct 25, 2018

Only Container::exec() left, which is... a bit tricky.

@abusch

This comment has been minimized.

Contributor

abusch commented Oct 28, 2018

@softprops I think this might be ready for another round of review, whenever you get a chance. I'm kind of torn about introducing a DockerFuture<T> type... It might simplify the signature a bit, and remove some usages of Either in the code, but not sure if it's worth the trouble. What do you think?

Also pinging @srijs in case you have suggestions/comments on my Hyper/Tokio/Future code :)

@abusch abusch changed the title from [WIP] Async api to Async api Oct 28, 2018

@abusch

This comment has been minimized.

Contributor

abusch commented Nov 13, 2018

Hi @softprops, just wondering if you got a chance to look at this?

@softprops

This looks amazing. Thank you for all hard work on this. I'm really digging the TTY stuff. I actually think I have a project coming up soon that could use that.

@softprops

This comment has been minimized.

Owner

softprops commented Nov 14, 2018

Crap there's one tiny merge conflict that crept in. Can you merge master to resolve real quick and I'll merge. Thanks for your patience on this

@abusch

This comment has been minimized.

Contributor

abusch commented Nov 14, 2018

No worries. I'll resolve the conflict shortly on my commute home :)

@abusch

This comment has been minimized.

Contributor

abusch commented Nov 14, 2018

All checks are green!

@softprops softprops merged commit 79d65c2 into softprops:master Nov 14, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@softprops

This comment has been minimized.

Owner

softprops commented Nov 14, 2018

Awesome work. I'm still on vacation but I'll try to get a release out soon. I kind of gave up on release notes for this one. I'm going to try doing releases more often for this repo to ease that burden

@schrieveslaach

This comment has been minimized.

Contributor

schrieveslaach commented Dec 3, 2018

@abusch, I tried to use the new async API and I'm wondering how is it possible to collect data. For example, I want to load some information from some containers and provide a REST response. Could you provide any examples?

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