Conversation
Tokio-core seems to be deprecated so remove it and use Tokio's Runtime instead.
dylanmckay
left a comment
dylanmckay
left a comment
There was a problem hiding this comment.
left a few minor comments, mostly typos
Double checked all newly-added dependencies and it looks like they all still refer to the most up-to-date version of the package (it's been a while since the patch was first written).
Looking nice.
| } | ||
| } | ||
|
|
||
| /// Creates a new docker instance for a dockr host |
There was a problem hiding this comment.
s/dockr/docker, it's potentially my typo originally though
| runtime: RefCell<Runtime>, | ||
| host: String, | ||
| }, | ||
| /// TCP/TLS.w |
| } else { | ||
| None | ||
| fn get_error_message(res: Response<Body>) -> Option<String> { | ||
| let chunk = match res.into_body().concat2().wait() { |
There was a problem hiding this comment.
Most of the other wait calls have been replaced with self.runtime().block_on(req) - should this one be too?
| use std::io::{Read, Write}; | ||
| use tokio::runtime::Runtime; | ||
|
|
||
| trait InteractiveStream : Read + Write { } |
There was a problem hiding this comment.
I suspect this might be left over from when I first split this PR out from some related shiplift changes, it doesn't look used.
|
Thanks for the review! I think I've addressed all the comments. |
|
Perfect, those four comments have all been fixed |
|
@softprops what say you? Do you think this can be merge? |
|
It's been a while. I'm going to try to fold as much as I can into a new release including this |
|
Can you fix up the remaining merge conflicts. I'll merge after and try to roll up a release |
|
Alright, I've merged master in this branch and resolved the conflicts. It builds and the tests pass both with and without the feature. |
| | StatusCode::CREATED | ||
| | StatusCode::SWITCHING_PROTOCOLS => { | ||
| let chunk = | ||
| self.runtime().block_on(res.into_body().concat2())?; |
There was a problem hiding this comment.
I don't think this correctly handles the case where a stream never ends; when reading from an endpoint like /events the return is never hit. Instead of storing data in memory and returning a Cursor over a vec, this method should instead return a Read that blocks for u8 as they come in.
There was a problem hiding this comment.
Yes, I suspect you're correct... The whole wrapping of Hyper's async API in a synchronous API is quite clunky unfortunately :( Ideally this would return a Stream<Vec<u8>> or similar, and I'm slowly working towards this in a branch but I'm not there yet... I'm not sure how to fix this until then, so I'm open to ideas :)
4 participants
This builds upon @dylanmckay's branch that did most of the work. I've bumped hyperlocal to 0.6, removed the dependency on
tokio_core, and now store an instance ofRuntimeinTransportinstead of creating a new one for every request. I'm not sure which of these changes did it, but it fixes the hangs I was having with this branch.