Skip to content
This repository was archived by the owner on Jun 21, 2020. It is now read-only.

Tokio 0.2 #43

Closed
wants to merge 4 commits into from
Closed

Tokio 0.2 #43

wants to merge 4 commits into from

Conversation

vorot93
Copy link
Contributor

@vorot93 vorot93 commented Aug 19, 2019

No description provided.

@vorot93 vorot93 requested a review from Nemo157 August 19, 2019 20:54
@vorot93 vorot93 force-pushed the tokio-0.2 branch 7 times, most recently from e66f87a to 7826e35 Compare August 22, 2019 15:02
@Nemo157
Copy link
Contributor

Nemo157 commented Aug 22, 2019

Did you try using hyper::service::{make_service_fn, service_fn} for this? At first glance they look like might be able to handle all the wrapping that is necessary (I may even have been able to use them in my first implementation of this, but didn't notice them in the docs then).

If they don't work, then this code looks fine and works in my small project. Main thing I'm not sure about is depending on git repos, I'll leave making a call on that up to other maintainers that are more invested in this crate.

@secretfader
Copy link
Contributor

In general, I prefer not to rely on Git repositories in released crates. (I don't mind the idea, in development, especially if part of testing practices for an ecosystem of crates.)

However, I think it makes sense to have a branch of http-service tracking changes in Tokio and Hyper as both projects prepare to expand usage of async/await. Does it make sense to, at the very least, depend on specific Git branches, or even better, revisions? I think so. The question then is whether these changes belong on master, either now or in the immediate future, and I'll need to consider that further before forming an opinion.

@vorot93
Copy link
Contributor Author

vorot93 commented Aug 24, 2019

@Nemo157 suprisingly, make_service_fn turned out to be more unwieldy than WrapHttpService. Maybe somebody else needs to take a look.

@secretfader Hyper is the prime example of how to proceed. Right in the master branch it switched to git version tokio and then switched back to 0.2 alphas as they got released.

@secretfader
Copy link
Contributor

If we're able to update the entire rustasync ecosystem at once (and maintain those updates, as new Tokio releases drop), then I'm in favor of merging these updates.

I'm not sure how rapidly new Tokio releases will land, but I believe it could be useful to keep these updates in a separate branch until we're reasonably sure that we can keep up with the release cadence of dependencies. An out-of-date or broken master branch is a major frustration, and one I'd like to avoid if possible.

@vorot93
Copy link
Contributor Author

vorot93 commented Aug 29, 2019

Yes, we should be able to keep up especially since http-service is only really used by Tide. See the companion PR at http-rs/tide#307

@abonander
Copy link

Is this ready to merge? The build is passing.

@secretfader
Copy link
Contributor

LGTM. I'd like to see this merged too. @Nemo157 @yoshuawuyts, thoughts?

@dvic
Copy link

dvic commented Sep 30, 2019

Just as a side note: tokio 0.2.0-alpha.5 has been released on Sep 19th, not sure if it contains any breaking changes.

@vorot93
Copy link
Contributor Author

vorot93 commented Oct 1, 2019

Please note that this PR is not compatible with the latest hyper alpha due to the new Accept trait. I could use some help with it.

@vorot93 vorot93 force-pushed the tokio-0.2 branch 2 times, most recently from d564fab to b389e71 Compare October 1, 2019 15:19
@secretfader
Copy link
Contributor

secretfader commented Oct 1, 2019

tokio-0.2.0-alpha.6 was published yesterday, which creates another issue to be addressed. Before, I wasn't sure if we needed to merge this as-is or wait for updates. Now I know waiting for updates is the most prudent of available choices.

I wish that I had more time to contribute. I expect others are in a similar bind. In my opinion, it's fine to let this PR sit until we can update to the latest dependencies and APIs (which I hope will also have less frequent churn by then).

@vorot93
Copy link
Contributor Author

vorot93 commented Oct 2, 2019

@secretfader Speaking of features at hand, the said Accept trait is not going away any time soon. This is tech debt that needs to be closed in any case.

@jakobhellermann
Copy link

regarding the Accept trait; it looks like a Stream can be converted using hyper::server::accept::from_stream when the stream feature is enabled. Does this help @vorot93?

@vorot93 vorot93 force-pushed the tokio-0.2 branch 2 times, most recently from ab350a2 to 69339ab Compare October 7, 2019 14:28
@vorot93
Copy link
Contributor Author

vorot93 commented Oct 7, 2019

I have made the minimum glue but had to sacrifice the customization incl. choosing non-Tokio runtime.

.await?;

let (parts, body) = rsp.into_parts();
let body: hyper::Body = body.into_vec().await?.into();
Copy link

Choose a reason for hiding this comment

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

Shouldn't the body be wrapped into a compatible stream for hyper::Body instead of converting it into a Vec? I see for that we need the unstable-stream feature from hyper.

@Nemo157
Copy link
Contributor

Nemo157 commented Oct 26, 2019

dbf2df3 retains support for alternative runtimes (though I haven't tested it other than checking the example compiles).

@yoshuawuyts
Copy link
Member

This has been superseded by #49 and #48. Thanks!

@yoshuawuyts yoshuawuyts deleted the tokio-0.2 branch November 14, 2019 13:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants