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

[WIP] Update to hyper 0.12 #88

Closed
wants to merge 4 commits into from

Conversation

dylanmckay
Copy link
Contributor

@dylanmckay dylanmckay commented Aug 6, 2018

I've updated shiplift to Hyper 0.12.

The biggest problem was the hyperlocal dependency, which hasn't been updated in several months.

One issue caused by this is the fact that hyper::Uri cannot parse unix socket URIs starting with a /. "unix:///foo".parse().unwrap() will always trigger a parse error.

In order to update this crate to 0.12, hyperlocal must also be updated to hyper 0.12. I've done that in softprops/hyperlocal#11, which has not landed yet.

This merge request cannot be merged until the hyperlocal PR has been merged and released to crates.io.

I also added support for the 304 Not Modified status code, which I encountered from Docker whilst creating containers.

@dylanmckay dylanmckay force-pushed the update-to-hyper-0.12 branch 3 times, most recently from d692afc to b3a57cc Compare August 7, 2018 06:42
@softprops
Copy link
Owner

Wow awesome! I'll dig into both these changes starting with hyperlocal. This is a hide weight off my shoulders

@dylanmckay
Copy link
Contributor Author

dylanmckay commented Aug 10, 2018

Wow awesome! I'll dig into both these changes starting with hyperlocal. This is a hide weight off my shoulders

No problem, thanks for writing such a great library!

Here's a few extra notes on this PR

  • We should probably remove the ownedtokio_reactor::Core from Transport, or at least allow custom cores to be set
    • Because you cannot nest cores, this means that all Docker API calls made using this crate will fail with a tokio runtime error the moment a future spawned by one core attempts to spawn another future on a different core.
    • https://stackoverflow.com/questions/50644880/cannot-recursively-call-into-core-when-trying-to-achieve-nested-concurrency
    • This makes using shiplift in networking software running tokio impossible
    • One idea off the top of my head is to modify Transport and Docker so that a core (or maybe a Handle or Remote?) can be passed in when building the Docker object. Perhaps leverage the builder pattern and default to creating an owned core when a custom core is not specified.
  • I'm only testing over TCP on my machine, we should test Unix sockets before merging
  • I've got another WIP branch adding support for TCP upgrades and container streaming. This makes it possible to interactively stream stdout/stderr (possibly multiplexed) and stdin. My usecase is to spawn docker containers and run isolated, interactive shell sessions. I am using TCP because Docker does not support upgrades over Unix sockets. I'll probably push that PR up once this one lands so I don't have to rebase lots.

@softprops
Copy link
Owner

heads up published hyperlocal with the hyper version upgrades in hyperlocal@0.5.0

@abusch
Copy link
Contributor

abusch commented Aug 28, 2018

Hi, is there anything I can do to help this move forward? In particular, I'm interested in the handling of the 304 status code which I've run into as well...

@abusch
Copy link
Contributor

abusch commented Aug 28, 2018

Also, I gave this branch a try but my app hangs on startup, presumably when trying to connect to the docker daemon over a Unix socket. I haven't investigated much yet but I'll see if I can figure out what's wrong.

@abusch
Copy link
Contributor

abusch commented Sep 6, 2018

So I did a bit of digging. The "good" news is I can reproduce it simply by running the shiplift's examples, i.e. cargo run --example images hangs (although for some reason cargo run --example containers works for me, which is a bit puzzling).

I think I've tracked down the code that hangs to this line: 1b8e837#diff-ec7d691b5cafdc631a44bf41efa29e95R114

The wait() in particular triggers all kinds of alarm bells in my head (especially reading the documentation), but I don't know enough about hyper/tokio/futures yet to know what's wrong with it.

The fact that it works for cargo run --example containers makes me think that it might have something to do with the size of the response, i.e. the problem occurs if there is more than one Chunk to stream from the response...

I'll keep investigating a bit, but any help is welcome :)

@dylanmckay
Copy link
Contributor Author

@abusch sorry, I hadn't seen your comments on here earlier!

@abusch has opened #91 which subsumes this. It looks like they've probably fixed the hanging.

I'm gonna close this in favour of #91

@dylanmckay dylanmckay closed this Sep 24, 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