start tutorial #1
Conversation
|
Wow -- this is beautiful! I'll take a closer look a bit later but just want to say thanks for writing this up! <3 |
|
sigh, I'd love mdbook to support asciidoc... |
|
FWIW, please continue, I'll find a good way to integrate it into our material. |
src/main.rs
Outdated
| while let Some(stream) = incoming.next().await { | ||
| let stream = stream?; | ||
| println!("Accepting from: {}", stream.peer_addr()?); | ||
| let _handle = task::spawn(client(broker_sender.clone(), stream)); |
There was a problem hiding this comment.
I think it'd be best to unwrap the error here so that a panic crashes the program instead of silently continuing, perhaps like this:
let broker_sender = broker_sender.clone();
task::spawn(async move {
client(broker_sender, stream).unwrap();
});There was a problem hiding this comment.
I feel like this is the case where we should not crash. If a single client fails due to some IO error (which might be on the client's side!) other clients should not be affected.
I am planing to write about graceful shutdown next (so that we can reap errors at least in the end) and then about client's disconnections (which will bring us to futures_unordered and just in time error reaping).
There was a problem hiding this comment.
Fair enough! But, at the very least, surely we should at least print an error on the screen? Maybe like this:
let broker_sender = broker_sender.clone();
task::spawn(async move {
if let Err(err) = client(broker_sender, stream) {
eprintln!("{:?}", err);
}
});There was a problem hiding this comment.
Yeah makes sense, especially if we gloss over the fact that eprintln! is blocking :-)
I've actually got a better idea of how to handle disconnections overnight. Instead of futures unorderned and waiting for the tasks to join, we'll just use channels. I think that's what Go does: IIRC, there's no way to wait until go routine is joined, you can only wait until it sends an "I am about to die" message over a channel.
There was a problem hiding this comment.
That's the strategy that I usually use.
Btw, considering that println! and eprintln! are blocking, should we provide our own versions of them?
There was a problem hiding this comment.
Btw, considering that
println!andeprintln!are blocking, should we provide our own versions of them?
Maybe, but I'm not sure what's the best way of approaching that.
There was a problem hiding this comment.
cough is it possible to extend clippy by using it as a library? (async-clippy)
I see a problem here: is it async_std::println!("fooo").await or async_std::eprintln!("foo"). In general, I think we should encourage using log! here anyways.
There was a problem hiding this comment.
Thinking further about this, println and friends in an async block is probably a good clippy lint to suggest.
|
Pushed the "chapter" about handling disconnections. Am I overthinking this? |
|
@matklad I don't think you are overthinking this, but I do get the feeling that multiple chapter start to make sense :). |
|
For the purpose of this repository, can we make the tutorial the README? |
| let mut lines_from_server = futures::StreamExt::fuse(reader.lines()); | ||
|
|
||
| let stdin = BufReader::new(stdin()); | ||
| let mut lines_from_stdin = futures::StreamExt::fuse(stdin.lines()); |
There was a problem hiding this comment.
This is pretty unfortunate: I need to manually .fuse the streams to make them work in select, and, unlike async-rs/async-std#13, these streams are not fused.
Note that I have to use UFCS to avoid conflict with async_std::Stream, which seems like a big problem? I bet having both traits in scope would be pretty typical.
There was a problem hiding this comment.
That does seem like an important design issue, you might want to file this as a separate ticket.
No description provided.