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.

Sign up for GitHub

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

Jump to bottom

*: switch to futures 0.3 #447

Merged
merged 6 commits into from Apr 2, 2020
Merged

*: switch to futures 0.3 #447

merged 6 commits into from Apr 2, 2020

Conversation

BusyJay
Copy link
Member

@BusyJay BusyJay commented Mar 23, 2020

Close #237.

Async trait doesn't seem to be supported yet. There is a solution, but I would rather wait sometime and check the community adoptions.

@BusyJay
*: switch to futures 0.3
aaefbed 
Signed-off-by: Jay Lee <BusyJayLee@gmail.com>
This was referenced Mar 23, 2020
Closed
Closed
@BusyJay BusyJay requested a review from sticnarf March 23, 2020 08:09
porkbrain
porkbrain reviewed Mar 23, 2020
View reviewed changes
src/call/mod.rs Outdated
}
}
) -> Result<()> {
// `start_send` is sopposed to be called after `poll_ready` returns ready.
Copy link

Choose a reason for hiding this comment

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

Typo: sopposed -> supposed

@BusyJay
address comment
9c6d280 
Signed-off-by: Jay Lee <BusyJayLee@gmail.com>
@BusyJay
Merge branch 'master' into use-futures-0.3
09ddf0d 
Signed-off-by: Jay Lee <BusyJayLee@gmail.com>
nrc
nrc reviewed Mar 25, 2020
View reviewed changes
Copy link
Contributor

@nrc nrc left a comment

Choose a reason for hiding this comment

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

Nice! This is really cool, I love how much better it makes the examples look. I have some comments inline, but most are small and the PR mostly lgtm.

compiler/src/codegen.rs Show resolved Hide resolved
interop/src/bin/client.rs Outdated
use crate::grpc::{ChannelBuilder, ChannelCredentialsBuilder, Environment};
use crate::grpc_proto::util;
use clap::{App, Arg};
use futures::executor::block_on;
Copy link
Contributor

Choose a reason for hiding this comment

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

style nit: it is better to import the module and qualify the function with the module name, e.g., import futures::executor and use executor::block_on.

interop/src/bin/client.rs Outdated Show resolved Hide resolved
interop/src/bin/server.rs
@@ -64,5 +64,5 @@ fn main() {
}
server.start();

let _ = future::empty::<(), ()>().wait();
block_on(futures::future::pending::<()>());
Copy link
Contributor

Choose a reason for hiding this comment

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

Same thing here with tokio::main

src/client.rs
@@ -35,8 +34,7 @@ impl Client {
req: &Req,
opt: CallOption,
) -> Result<Resp> {
let f = self.unary_call_async(method, req, opt)?;
f.wait()
block_on(self.unary_call_async(method, req, opt)?)
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally, we wouldn't expose API that uses block_on since it means the caller can't use the function if they are using their own executor. Ideally we would only provide the async API unless the unary_call can be implemented without using async IO, the caller then only needs to use block_on. If we want to keep this function for back compat, then could you add to the doc comment that this function uses the default executor from the futures crate.

src/server.rs Outdated
@@ -569,7 +569,7 @@ impl Drop for Server {
// TODO: don't wait here
let f = self.shutdown();
self.cancel_all_calls();
let _ = f.wait();
let _ = futures::executor::block_on(f);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not ideal since it forces the user to use the default executor. Until we get async destructors, I think the best thing to do is to have the user manually call an async shutdown method rather than drop. This can be enforced at runtime by having a shutdown flag which is only set by the shutdown method and panicking in drop if that flag is not set.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm afraid it's not backward compatible. I will change it to only calling block_on when shutdown has not been called.

src/task/executor.rs
}

unsafe impl Sync for SpawnTask {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a comment for why this is safe (and why its required) please?

tests-and-examples/examples/route_guide/client.rs
let (sr, rr) = futures::join!(send, receive);
sr?;
rr?;
Ok(())
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be sr.and(rr)?

tests-and-examples/examples/route_guide/client.rs
Ok(())
}

fn main() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could use tokio::main rather than async_main

src/call/mod.rs Outdated
Ok(Async::NotReady) => {}
if let Some(close_f) = &mut self.close_f {
if let Poll::Ready(_) = Pin::new(close_f).poll(cx)? {
// don't return immediately, there maybe pending data.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// don't return immediately, there maybe pending data.
// Don't return immediately, there may be pending data.

@BusyJay
address comment
cbf66f7 
Signed-off-by: Jay Lee <BusyJayLee@gmail.com>
@BusyJay
Copy link
Member Author

BusyJay commented Mar 31, 2020

PTAL

sticnarf
sticnarf reviewed Apr 1, 2020
View reviewed changes
Cargo.toml Outdated
@@ -42,3 +42,6 @@ debug = true

[badges]
travis-ci = { repository = "pingcap/grpc-rs" }

[patch.crates-io]
grpcio-compiler = { path = "compiler", version = "0.5.0", default-features = false }
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing \n at end of file

src/call/mod.rs Show resolved Hide resolved
proto/Cargo.toml Outdated
@@ -28,5 +28,4 @@ lazy_static = { version = "1.3", optional = true }

[build-dependencies]
protobuf-build = { version = "0.11", default-features = false }
grpcio-compiler = { path = "../compiler", version = "0.5.0", default-features = false }
walkdir = "2.2"
walkdir = "2.2"
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing \n at end of file

src/call/server.rs Show resolved Hide resolved
@BusyJay
Merge branch 'master' into use-futures-0.3
84c639a 
Signed-off-by: Jay Lee <BusyJayLee@gmail.com>
@BusyJay
address comment
7d73c44 
Signed-off-by: Jay Lee <BusyJayLee@gmail.com>
nrc
nrc approved these changes Apr 1, 2020
View reviewed changes
Copy link
Contributor

@nrc nrc left a comment

Choose a reason for hiding this comment

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

lgtm, thanks for the changes!

sticnarf
sticnarf approved these changes Apr 2, 2020
View reviewed changes
Copy link
Contributor

@sticnarf sticnarf left a comment

Choose a reason for hiding this comment

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

LGTM

@BusyJay BusyJay merged commit fd66ec7 into tikv:master Apr 2, 2020
@BusyJay BusyJay deleted the use-futures-0.3 branch April 2, 2020 04:47
BusyJay added a commit to BusyJay/grpc-rs that referenced this pull request May 7, 2020
@BusyJay
Revert "*: switch to futures 0.3 (tikv#447)"
c8a69ed 
This reverts commit fd66ec7.

Signed-off-by: Jay Lee <BusyJayLee@gmail.com>
@Maher4Ever Maher4Ever mentioned this pull request May 14, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@porkbrain porkbrain porkbrain left review comments

@nrc nrc nrc approved these changes

@sticnarf sticnarf sticnarf approved these changes

@overvenus overvenus Awaiting requested review from overvenus

@hunterlxt hunterlxt Awaiting requested review from hunterlxt

Assignees
No one assigned
Labels
Enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add support for async/await syntax
4 participants
@BusyJay @nrc @porkbrain @sticnarf