Update hyper

[danieleades]

What did you implement:

port of shiplift to futures-0.3

mostly complete, aside from the examples and a couple of methods

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

This is a change to support async/await syntax. It is a BREAKING CHANGE

[FedericoPonzi]

What's the status of this PR? It looks like travis is failing because async/await are unstable.

[danieleades]

There's been a lot of churn in the last few weeks, this will need a lot of reworking. Might get some time to take a look in the next couple of weeks

[danieleades]

Still working on this, slowly!
It's a big refactor. I've used futures::stream, rather than Tokio::stream as adoption has been greater here and the stream combinators are far far more mature. This also rules out whole swathes of Tokio API, so there's a few things I'm having to reimplement.
It's not all bad news- it's looking far tidier, and I'm clearing out all of the manual Future, Stream, and AsyncRead implementations in favour of returning 'impl traits'

It doesn't build, I haven't tested anything, and I've got loads left to do. Will try and push a WIP in the next few days.

May also need some help on a couple of things, and would certainly appreciate some code reviews. Any volunteers?

[FedericoPonzi]

Sure, just push commits and ask questions/reviews. I'll pick them up as much as I can, and other people might join too.

[danieleades]

that's the painful stuff done. There was a surprising amount of yak-shaving went into this.

• i've removed a couple of problematic endpoints, these will need to re-added
• there's a couple of APIs that might need some discussion. easiest way to check which ones feel weird is probably to update all the examples (i've done a couple)
• now that it actually builds, the compiler is a little more helpful, so time now for a big refactor.

This is still very much a work in progress

[danieleades]

I'd appreciate some feedback on the ergonomics of return pinned vs unpinned streams.

On the one hand, returning Unpin streams makes them far easier to work with for the consumer. It does mean an extra allocation in many return functions to create Pin<Box<T>>s.

On the other hand, it seems perhaps more "Rusty" to avoid the extra allocation and let the consumer pin the stream if they need to

Or maybe there's a far easier way to return Unpin streams without having to box everything that I'm missing

[danieleades]

The checks are now passing

before we can start to talk about merging-

• where are the external tests in some of the examples being run? (@softprops?)
• refactor and tidy
• reimplement Container::attach()
• re-enable and implement unix-socket feature
• resolve question of whether returned streams should be Unpin or not
• split futures dependency
• fix Container::copy_from() implementation and example
• update changelog
• update readme

[danieleades]

doing some refactoring now.

the Options structs are a little unusual-

• they use a builder API to determine which fields can be set
• but internally they are loosely typed using serde_json::Values?
• they manually implement serialisation methods, even though serde is already a dependency
• they do this using a parse_from(...) method which takes self, but doesn't use it. this is then called from other methods, passing in references to self as the other arguments.

impl ContainerOptions {
    /// return a new instance of a builder for options
    pub fn builder(name: &str) -> ContainerOptionsBuilder {
        ContainerOptionsBuilder::new(name)
    }

    /// serialize options as a string. returns None if no options are defined
    pub fn serialize(&self) -> Result<String> {
        serde_json::to_string(&self.to_json()).map_err(Error::from)
    }

    fn to_json(&self) -> Value {
        let mut body_members = Map::new();
        // The HostConfig element gets initialized to an empty object,
        // for backward compatibility.
        body_members.insert("HostConfig".to_string(), Value::Object(Map::new()));
        let mut body = Value::Object(body_members);
        self.parse_from(&self.params, &mut body);
        body
    }

    pub fn parse_from<'a, K, V>(
        &self,
        params: &'a HashMap<K, V>,
        body: &mut Value,
    ) where
        &'a HashMap<K, V>: IntoIterator,
        K: ToString + Eq + Hash,
        V: Serialize,
    {
        for (k, v) in params.iter() {
            let key_string = k.to_string();
            insert(&mut key_string.split('.').peekable(), v, body)
        }
    }
}

strange, no?

strongly typing the options is probably a battle for a different day. hard to resist large-scale refactors since this pull request already touches on so much code.

[FedericoPonzi]

doing some refactoring now.

the Options structs are a little unusual-

• they use a builder API to determine which fields can be set
• but internally they are loosely typed using serde_json::Values?
• they manually implement serialisation methods, even though serde is already a dependency
• they do this using a parse_from(...) method which takes self, but doesn't use it. this is then called from other methods, passing in references to self as the other arguments.

impl ContainerOptions {
    /// return a new instance of a builder for options
    pub fn builder(name: &str) -> ContainerOptionsBuilder {
        ContainerOptionsBuilder::new(name)
    }

    /// serialize options as a string. returns None if no options are defined
    pub fn serialize(&self) -> Result<String> {
        serde_json::to_string(&self.to_json()).map_err(Error::from)
    }

    fn to_json(&self) -> Value {
        let mut body_members = Map::new();
        // The HostConfig element gets initialized to an empty object,
        // for backward compatibility.
        body_members.insert("HostConfig".to_string(), Value::Object(Map::new()));
        let mut body = Value::Object(body_members);
        self.parse_from(&self.params, &mut body);
        body
    }

    pub fn parse_from<'a, K, V>(
        &self,
        params: &'a HashMap<K, V>,
        body: &mut Value,
    ) where
        &'a HashMap<K, V>: IntoIterator,
        K: ToString + Eq + Hash,
        V: Serialize,
    {
        for (k, v) in params.iter() {
            let key_string = k.to_string();
            insert(&mut key_string.split('.').peekable(), v, body)
        }
    }
}

strange, no?

strongly typing the options is probably a battle for a different day. hard to resist large-scale refactors since this pull request already touches on so much code.

Yes, maybe @softprops know if there is a reason behind this decision. Is there?
What if we merge this pr in a "future-03" branch, and keep working on that branch? Otherwise, if this is ready "enough", I would give it a review (maybe also by @softprops ) and bump the version of Shiplift also on crates.io. Then we could update stuff (like strongly typing options) in diifferent feature branches and push them as minor versions. WDYT?

[softprops]

I'll take a look this weekend. Context for the off serialization schemes. This crate predates serde's existence and stablization of proc macros which enabled deriving serialization on stable rust. Over time this crate has incorporated pulls from contributors that add components which get it part way there but I believe there needs to be a total overall of de/see realization that is only based on derives. I don't have the bandwidth for this now but would welcome such a pull. I don't think that serialization needs to be coupled to a change to the way io is managed.

[danieleades]

@softprops thanks for weighing in there. Agreed on all points

[softprops]

Just released 0.6.0. I think targeting 0.7 would be good for updating hyper and the async io deps

[danieleades]

Cool beans. Running into difficulties now concerning the split eco system between futures::AsyncWrite/Read and tokio:: AsyncWrite/Read. Tokio are making noises about reporting the futures traits, which would solve all my problems. I'll do a bit more investigation, but i might be blocked until that occurs

[schrieveslaach]

@danieleades, I took your branch and updated hyper to the latest release version (see here). Can you pull this commit into your branch so that it is included in this PR?

[danieleades]

@danieleades, I took your branch and updated hyper to the latest release version (see here). Can you pull this commit into your branch so that it is included in this PR?

Hi, thanks for this. I've actually been chipping away at this in a different branch and making much more drastic changes

• because i wasn't getting any responses from maintainers
• because there are a large number of design decisions in shiplift i've been enjoying tinkering with.
• I've also been slightly blocked upstream on port hyperlocal client to latest hyper hyperlocal#29
• i've had to add a compat layer between futures::io::Async* and tokio::io::Async*
• i wanted to explore using 'async builder' methods for the API endpoints (i'm really not a fan of the client.endpoint(Default::default()).await api)

it's been on hold for a little while as i have been very busy drinking alcohol and eating food these last couple of weeks, but i will try to push something soon.

[schrieveslaach]

@danieleades, thanks for the update. Until then I will work with my fork.

[schrieveslaach]

@danieleades, did you run into following issue and do you address it in your branch? I'm trying to inspect multiple containers with join_all:

        let docker = Docker::new();
        let containers = docker.containers();
        let futures = containers
            .list(&list_options)
            .await?
            .iter()
            .map(|container| containers.get(&container.id).inspect());

        let mut container_details = MultiMap::new();
        for details_result in join_all(futures).await {
            let details = details_result?;
            // …
        }

and the compiler tells me:

    |
419 |             .map(|container| containers.get(&container.id).inspect());
    |                              -----------------------------^^^^^^^^^^
    |                              |
    |                              returns a value referencing data owned by the current function
    |                              temporary value created here

I think that docker.containers() should return an instance that is not holding a reference to docker. It should rather contain a copy of docker.

@softprops, what do you think about this PR (potentially including 9f4c6c09).

[danieleades]

Actually, I did neither. I created an http client object, which gets passed down from the docker client to the containers client in an Arc.

[danieleades]

for those interested, i've pushed a branch with some of the refactoring i've done.

it's a work in progress, but shows some changes that i think should be merged into master
https://github.com/danieleades/shiplift/tree/2020

• update everything to futures 0.3 and futures::Async[Write/Read] (i added a compat layer for interop with tokio::Async[Write/Read])
• create a HttpClient object, and move all the request helpers currently on Docker into this object
• I also created a Request object so as to have a builder API for constructing requests, rather than the explosion of methods which currently exist (very similar to Reqwest. in fact, if Reqwest ever supports UDS, shiplift should migrate to this, rather than reinventing the wheel
• the HttpClient is passed around between clients as an Arc<HttpClient>. This eliminates one of the lifetimes from the clients, and allows the other clients to outlive the Docker client.
• the tarball implementation has to go- it's currently a mess and it's a completely synchronous module- in any server-type implementation this would block the entire process. I've not looked at this yet, but i'd suggest using https://github.com/dignifiedquire/async-tar
• the custom stream decoding can be replaced with tokio_util::codec. it will need the compat layer until tokio gets around to re-exporting futures::AsyncWrite/Read. This also applies to the line-encoded streams
• the options should be re-implemented using static structs and serde. i've done this on a couple of endpoints. These options should also carry a reference to the HttpClient and implement Future or IntoFuture (if that gets re-merged). This will allow a builder API for creating the options, which can then be awaited directly. would be a lot nicer than passing in Default::default() all the time. (see https://blog.yoshuawuyts.com/async-finalizers/)
• a version of the image::build endpoint should be added which accepts an impl AsyncRead of the directory tarball, so it can be used over a network (when the directory itself isn't local)
• for the love of all that is good and holy break the clients into separate files.

I would absolutely say that the largest blocker to refactoring this crate is the absence of tests. Any effort to add some test coverage to the endpoints should be prioritised.

I'm unlikely to take this further to be honest, it's a lot of work to refactor every endpoint in one hit, no matter how badly it needs it.
It's way too many changes for a pull request, so i'm going to have to try and rescope and find an incremental adoption approach to the changes. I suggest (@softprops) that a branch is made in the main repo for a refactor, and then people can create pull requests gradually on this branch until it reaches feature-parity with master.

I would be more than happy to contribute to this effort.

In the meantime, i sincerely hope others can find some inspiration in the bits and pieces i've hacked together so far, and i'll be more than happy to help with any code reviews.

I may get time to do some additional work on this, but i'm getting dragged off on other projects.

apologies this isn't a more positive update!

[thomaseizinger]

@danieleades thanks for all of your hard work on this! I've been eagerly watching the progress :)

In regards to async-tar: This brings in all of async-std (unfortunately). I am not aware of a tokio-based async implementation of tar.

[danieleades]

Async std and Tokio are largely interopable now that Tokio is re-exporting futures::stream. It is not yet re-exporting the AsyncRead and AsyncWrite traits, but the compat solves that.
Dependency bloat is your enemy here, but to be honest both libraries are now so heavily feature-gated, you're not paying for much more than you need.

[softprops]

@danieleades do you have any intent to push this forward? If not I might take a shot at it.

[danieleades]

@danieleades do you have any intent to push this forward? If not I might take a shot at it.

negative. Though I sincerely hope there's a lot of code here you can reuse!

In all honesty, I did most of the work. But it was such a big refactor and without any tests i had no way of knowing if it actually worked.
It might be good to think about some test coverage before doing this refactor

[softprops]

Thanks for your work on this. I'll try and pick up where you left off

[schrieveslaach]

@softprops, I used an intermediate version of this PR in PREvant with an updated version of hyper. This version works fine and we use PREvant every day at work which involves permanent usage of the Docker API through shiplift.

[elihunter173]

@softprops Other people have echoed similar thoughts, but would it be possible to merge this PR essentially as is into a branch like async-await and then accept PRs on that branch? I personally would like to help on this transition but just having one PR for this big of a change makes it harder because there's no super-clear way to contribute to this existing PR.

And, if the async-await branch is mentioned in the README it would allow people to use the upgrade while it's being refined and tested. Like @schrieveslaach mentioned he's using a project that uses his own version of this PR and in the meantime I'm using that same branch, but I think having an "official" way to do that would be good.

Also, I'm a newcomer to this project/PR (my project that uses this is just a week old!), so I just wanted to say thank you to everyone who contributed to the project and this PR for making such an ergonomic docker API.

[elihunter173]

I made a branch on top of @schrieveslaach's and brought it up to date with master and got all tests and checks to pass. I'm using it successfully in my project, but I'm not exercising all paths in that project.

[softprops]

sry folks the only blocker here for me is syncing with master to deal with the merge conflicts. If anyone can open a pr or help sync this pr. I'll be happy to merge so work can move forward

[elihunter173]

My branch should work as that. I'll open a PR

[softprops]

many thanks @elihunter173 for pushing this forward!

[schrieveslaach]

@elihunter173 thanks for effort!