move to tokio, fix one broken example #101
Conversation
|
Hi, thanks for the PR! Skimming through the diff, it looks like much more than fixing an example -- could you describe the changes you made and why they're important? |
|
The PR consists of two parts
[1] https://github.com/tokio-rs/tokio-core |
|
@zslayton Any update? If there's a problem, I'm willing to submit a new patch, for example removing |
|
@yjh0502 Sorry for the delay, I've been traveling for the last week. I'll get a chance to review this over the weekend. |
src/session.rs
Outdated
| .as_mut() | ||
| .map(|t| t.poll()) | ||
| .unwrap_or(Ok(Async::NotReady))?; | ||
| .unwrap_or(Ok(Async::NotReady)) | ||
| .map_err(|_e| IoError::new(ErrorKind::Other, "timer"))?; |
There was a problem hiding this comment.
I'm not very familiar with tokio_timer. Does it not provide an idiomatic return value to represent timeouts?
There was a problem hiding this comment.
There are two kinds of error, shutdown when there's no executor and at_capacity when timeout delay is too large to be handled (longer than 2 years in default timer).
tokio-timer has it's own error type, but it should be wrapped to std::io::Error because Session::Error is std::io::Error. We can change error string more specifically to help troubleshooting.
src/session.rs
Outdated
| @@ -468,7 +476,8 @@ impl Stream for Session { | |||
| let txh = self.state.tx_heartbeat_timeout | |||
| .as_mut() | |||
| .map(|t| t.poll()) | |||
| .unwrap_or(Ok(Async::NotReady))?; | |||
| .unwrap_or(Ok(Async::NotReady)) | |||
| .map_err(|_e| IoError::new(ErrorKind::Other, "timer"))?; | |||
There was a problem hiding this comment.
Same question RE: an error type for timeouts.
| @@ -0,0 +1,153 @@ | |||
| extern crate env_logger; | |||
There was a problem hiding this comment.
This example is no longer its own subcrate, which I'm ok with but it makes it difficult to tell where the diffs are.
examples/send_and_subscribe.rs
Outdated
| session: Session, | ||
| destination: String, | ||
| session_number: u32, | ||
|
|
There was a problem hiding this comment.
This extra whitespace is unneeded.
| } | ||
| } | ||
|
|
||
| impl Future for ExampleSession { |
There was a problem hiding this comment.
This stomp library has already changed its underlying implementation quite a few times. It's been singlethreaded, multithreaded, mio-based, tokio-core-based, and now tokio-based. It's not stable yet, so we can still make changes to the APIs. However, most of the changes in each migration have been implementation details. It's best if we don't require the end users of the API to change their code unless they have to.
In this example, the end user should only have to implement the methods on the Handler trait that they're interested in. If there's new, tokio-specific code that needs to be written for the Handler trait to work properly (for example, implementing Future), that should be an implementation detail handled by the library itself.
There was a problem hiding this comment.
ReceiptHandler/Handler is removed before I started to work on the library.
|
Thanks for the review! I appended a commit to handle issues that addressed on the review. Primary goal of those changes is make
I didn't sent a PR for those changes yet because I believe those changes are quite opiniated. How do you think of sending PR for those changes? |
|
Sorry to hijack this PR thread, but is there an effort to get modern futures/tokio support for this library and is the work done to-date (including this PR) part of that effort? Or is the library mostly OBE? |
|
Hi, thanks for drawing my attention back to this PR. It had fallen off my radar. This PR is the only work happening on this library and I need to find the time to land it. AFAIK there isn't another |
|
It would seem to be still worthwhile :) |
|
@madbonkey That it would! I guess STOMP is not a protocol in too much demand with those writing services in Rust. |
|
Seems true enough - I'd be happy to help out if there's something to do! A good library for STOMP would be nice to have, not so much for services/servers, but there's a lot of existing systems around using it and a robust client library would have an audience I think. |
|
Mea culpa. It's been very difficult to find bandwidth for this project. 😞 Apologies to @yjh0502 for neglecting this. When I first created it I was using a variety of MQ servers at my day job and always had one lying around to test against. (Using STOMP was a nice cross-server compatibility option, and the protocol was much simpler to implement than AMQP.) Tokio has reached 1.0 since this PR was first opened (again, my fault!). Is anyone interested in upgrading this to the stable version? |
No description provided.