-
-
Notifications
You must be signed in to change notification settings - Fork 713
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
Upgrade to Tokio 1.0 #753
Upgrade to Tokio 1.0 #753
Conversation
5d2937c
to
cec8c56
Compare
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
cec8c56
to
e3c4c70
Compare
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job! Only two remarks :D
…tokio Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Tokio 1.0 has been released, so this PR should probably be updated to depend on that. |
I'm trying to upgrade to Tokio 1.0. |
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
The remaining issue is tokio-tungstenite. Trying to upgrade it to tokio 1.0, but it's easier said than done. |
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Forked tokio-tungstenite, to upgrade it to tokio 1, and depending on it, but build still fails. |
async-compression using bytes 1.0 should come out soon Nullus157/async-compression#111 (comment)
|
If anyone has tips wrt. fixing the build failures, I'm all ears :) |
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Co-authored-by: Paolo Barbolini <paolo@paolo565.org>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
src/filters/compression.rs
Outdated
@@ -56,7 +58,10 @@ pub struct Compression<F> { | |||
/// ``` | |||
pub fn gzip() -> Compression<impl Fn(CompressionProps) -> Response + Copy> { | |||
let func = move |mut props: CompressionProps| { | |||
let body = Body::wrap_stream(GzipEncoder::new(props.body)); | |||
let body = Body::wrap_stream(FramedRead::new( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did you prefer FramedRead
over ReaderStream
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jxs To be honest, I don't remember exactly, but it was probably from an example. Do you think ReaderStream is preferable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was inspired from the reqwest implementation https://github.com/seanmonstar/reqwest/blob/a19eb3419637856d6d42f55dff6e4790613b2933/src/async_impl/decoder.rs#L47
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @paolobarbolini, I can't bring myself to remember where I found this solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @paolobarbolini. @aknuds1 I asked because I was not sure either, Previously FrameReader
was the way to achieve AsyncRead
to Stream
conversion, but since ReaderStream
was introduced I would maybe suggest ReaderStream
as it's simpler, and recommended for this cases on tokio-util doc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@seanmonstar Do you have any opinion on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I converted from FramedRead into ReaderStream, ass @jxs recommended. I don't really know personally if there are any practical differences, but I can tell that the API is simpler (a second argument to FramedRead::new
can be dropped). I trust @jxs knows more about this than I do, and I'll let @seanmonstar be the judge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM thanks! 🎆
Test (uds, --features tokio/uds)
also needs to be renamed on the repository settings- should the
tokio_util
dependent filters (fs
,compression
) also be featured gated?
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work! I believe this is very close.
src/filters/ws.rs
Outdated
.on_upgrade() | ||
let mut resp = Response::new(self.ws.body); | ||
if let Some(on_upgrade) = self.ws.on_upgrade { | ||
resp.extensions_mut().insert(on_upgrade); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you already have the extension, you don't need to insert it here to remove it again. (on_upgrade
is the same as fut
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@seanmonstar I think I see what you mean, and try to simplify it to the best of my ability. Please have a look.
…tokio Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Should the return type of This following ... Bytes change tokio-rs/bytes#439, which removed EDIT: found the change |
It was replaced by Buf::copy_to_bytes @koivunej |
It appears 657f1c6 broke the build since
|
@Kiskae What's the command to reproduce your failure? |
@aknuds1 Calling
|
@Kiskae Aha, I see the problem, it's because src/test.rs contains public utilities and depends on hyper::Client. Guess I'll have to reintroduce the dependency on hyper's client feature @seanmonstar. |
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
i think another option could be to make it an optional |
Thanks @seanmonstar! 🎉 |
Co-authored-by: janpetschexain <58227040+janpetschexain@users.noreply.github.com> Co-authored-by: João Oliveira <hello@jxs.pt> Co-authored-by: Paolo Barbolini <paolo@paolo565.org> Co-authored-by: Muhammad Hamza <muhammadhamza1311@gmail.com> Co-authored-by: teenjuna <53595243+teenjuna@users.noreply.github.com>
Similar to @Urhengulas' PR #741, although more complete, this PR aims to upgrade to Tokio v1.0.
I can successfully run all tests with all features enabled:
cargo test --all-features
.This is my first time trying to contribute to warp, and I'm not super familiar with the internals, so please consider it a best effort :)
Fixes #725.