-
Notifications
You must be signed in to change notification settings - Fork 126
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
Tokio 1.0 #301
Tokio 1.0 #301
Conversation
Weird, it looks like |
Yeah, it looks like it can receive one message, and then poll_read_ready never wakes it up again. |
1b6853b
to
7e4bb17
Compare
Figured it out, I ran into the same issue as tokio-rs/tokio#3208 (comment) |
Hi and thanks for working on this! |
Oh, yeah, this is pretty much ready to merge if it passes review. |
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.
Ok! So I admit not knowing the inner details of Tokio and trusting that you do the right thing (or that other people will report bugs if you don't...).
So I think this is okay for merging given the two comments above are fixed/commented + that you squash all patches into a single commit.
Good work!
dbus-tokio/Cargo.toml
Outdated
libc = "0.2.69" | ||
tokio = {version = "0.2.4", features=["time", "macros", "io-driver", "rt-core", "rt-util", "rt-threaded", "stream"]} | ||
tokio = {version = "1.0", features=["time", "net"]} | ||
parking_lot = "0.11" |
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.
Unless there is a strong reason not to use std's Mutex, I would prefer to avoid additional dependencies.
dbus-tokio/src/connection.rs
Outdated
inner: ResourceErrorKind | ||
} | ||
#[derive(Debug)] | ||
enum ResourceErrorKind { |
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.
How about we make the enum public and skip "IOResourceError"? Potentially with an added non_exhaustive attribute.
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.
Also add derive(Clone)
btw
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.
Alright, wasn't sure whether I should add more public API surface area, since Box<dyn Error>
is pretty anonymous, but I guess it makes sense that a dbus IO reactor would fail with either an IO or a dbus error :)
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.
Oh, neither of the inner error types implement Clone.
And it's in! Thanks! |
Fixes #289
Currently it's compiling and I think it's roughly correct, but it's timing out while waiting for a response, even though the last call to poll_internal comes because read is ready.