Panic on send in channels by default?
#314
Comments
|
This is what 0.2.* did. I believe there was a discussion on this point, but I can't find it in the issue tracker. (I personally tend to agree with you.) |
To clarify, |
|
@matklad Ah right, yes, thank you. That is an important clarification! |
ghost
added
|
Hi there! The previous discussion @BurntSushi mentions is: crossbeam-rs/crossbeam-channel#61 A couple points on this issue:
To summarize, I think |
Looks like there's some kind of misunderstanding here. Today send blocks if the channel is full, and I don't suggest changing that. To be more precise, I suggest the following changes:
That's true, yeah, but I think crossbeam-channel should be the canonical channel library, and all others should copy API from it :) I suspect the current situation is primarily due to the fact that std's channel return a
A silly counterpoint is that
This is the point where I think we disagree the most :) From the code I write and the code I've seen, you just do (*) there are of course situations when Finally, move one stack frame up to the crossbeam-rs/crossbeam-channel#61 and crossbeam-rs/crossbeam-channel#39, I think the story went as follows:
However I think my proposal of To be clear, I don't want to argue a lot about this, I don't have too much experience with complex CSP/Actor systems, but the current proliferation of noisy unwraps seems unfortunate :-) |
Sorry, my mistake! :( I really had But anyway, I see now you weren't suggesting to change
Ah, nice! I haven't thought of that one. ;)
A bit off-topic, but this is one of the situations where I'd love the libs team to weigh in with their opinion. I want to make decisions that will overall make most people happy, but it's kinda overwhelming since there are so many disagreeing opinions. There was even a point in time where the Servo folks didn't like the interface of
Your analysis of the previous discussions is correct. And the reasoning for Now, the current API is perfect in many ways except two things might get tiresome: unwrapping on send and the sender/receiver split. Both are sometimes annoying, but not deal breakers, I think. If we automatically panic on But again, I do acknowledge the benefits of the proposed solution, it's just that there are tradeoffs and I don't feel confident it'd be a total success. :( |
That is totally fair! I myself only confident that the proposed API will work better for the kind of code I use crossbeam-channel for: a handful of communicating threads where the amount of data is small, but the error conditions are numerous and a clean tear-down is important.
👍 I feel it's important to do some kind of libz-blits or an RFC process for crossbeam-channel 1.0: this is one of the "interface" libraries. |
|
Let's see what the libs team has to say in Berlin, perhaps we can arrange some kind of crossbeam-channel 1.0 RFC this year :) |
|
@stjepang I won't be in Berlin, but I think you know my opinions on the matter. :-)
Is there more on this? Did the Servo folks give more detail about their concerns? |
|
@stjepang I added this ticket to the library team's agenda in Berlin, although I don't know if it will stick. You should bug people while you're there too. :-) |
|
Err, "forked" is not the right word - "wrapped" is a better one. Relevant comments:
A bit later they updated Thanks, I will go around bugging people! :) |
|
Good example from standard library: RefCell::(borrow|try_borrow): https://doc.rust-lang.org/std/cell/struct.RefCell.html#method.try_borrow |
|
We discussed |
|
My experience in Servo's use of let v = match chan.recv() {
Ok(v) => v,
Err(e) => return warn!("Receive failed ({}), e"),
};Another thing that might be worth discussing is that double-panic kills the entire process, so any panicking which runs during |
This is @asajeffrey would be cool to dig into these cases more. At least from There's a solution when you explicitelly code for shut-down, and it And there's a solution where you just rely on the ownership semantics Panic on send definitely nudges me towards architectures of the second Here's a specific case where I believe panicking helped me to write In rust-analyzer, I have a virtual file system, whose task is to The interface to watcher/scanner "actor" of VSF (which is a separate The file watching library I am using, However, I realized that the second So I refactored the code to make this helper thread and the VFS actor |
|
The matching code for send is either the same, or just There are two cases where we need to ensure no panicking. The first is in any code which may run in panic-handlers, to avoid double-panic killing the entire process. The second is in any code which does crash-handling, e.g. by displaying a crash page which invites the user to submit an issue report. In particular the constellation (which manages the content threads) and webrender (which is responsible for painting) have to be robust against panic, even if content threads are panicking. If I ruled the world then we'd have a |
|
(I'm posting this here at @BurntSushi's suggestion, based on our discussion on Reddit. Note that a lot of this comment is in the context of At Faraday, we mostly work with Let me back up and provide a bit more context. Most of the channels and streams that I see at work are ultimately writing to either: 1. A network socket with a
|
|
Thank you so much for these examples, @emk! I think they fully apply to crossbeam-channel. I think the most important case here is failable pipeline: Propagating
If we want to maintain unidirectional dataflow (that is, treat closed sender as a bug), a possible solution would be for while let Some(msg) = receiver.recv() {
match db.write(msg) {
Ok(()) => (),
Err(e) => {
ctx.error_sink.send(e);
break;
}
}
}
for msg in receiver {
log::error!("failed to write message to db, db is dead: {:?}", msg)
}The entity than can shut-down the pipeline at the beginning, by terminating the producer. In the database connection example we can be even fancier, and instead of logging all pending messages, we can send both the I do understand that this is a bit of "architecture astronaut" type of the solution, and I haven't actually tired in in practice, so I can't say if it'll work. Though remarkably the The second example is "panicking during shutdown". Here I think handling errors on send produces an objectively less than ideal solution: namely, messages are implicitly lost during shutdown. Maybe it's not a big deal, but maybe it's saving some user state to the database? The solution here seems to be to architecture shutdown sequence in such a way that you terminate This is a situation where "panicking by default on send" might nudge you to a better architecture: if dropping receiver while sender is alive is an error, than you'll have to arrange a clean shutdown sequence in topological order. If send returns a result, the path of least resistance is to add logging on send. Finally, the Go example also seems like the case where result on send is worse solution. Specifically, we have a parent coroutine and a child coroutine, and the problem is that, although parent has spawn the child, it's not interested in child's result for some reason. The proposed solution allows for the situation where the parent has finished, but the child is alive. That is, the child is effectively a zombie: it might hold onto some resources and it might do computations, but they are not interesting to anyone, and child will only know that once it attempts send. I think the better fix would be to make sure that parent just always joins the child in the end, even if it doesn't need the result anymore. All that said, I am now not sure that panicking on send is the right choice for crossbeam =) I personally am still convinced that this is the right model to think about channels, but I also see that there's a large existing body of code which relies on handling errors on send. |
|
Awesome exchange! Thanks so much to both of you for writing all that stuff out. It's a lot of work! I just wanted to respond to a teeny detail:
I think if this were the only concern, we could say, "in crossbeam-channel 1.0, if your code depended on send returning an error, you should use |
|
Thank you everybody for this wonderful discussion! I spent much of today writing async code, and I encountered the first time where I actually wanted to write But I still believe there other perfectly reasonable use cases where I'd like to respond to everybody's comments in more detail, with examples of each use case, but it may have to wait a day or two until I merge this branch, which uses @BurntSushi's So I hope to have more to say very soon, once I get this example hooked up and passing integration tests! |
|
So I had a very busy week with channels and streams, and everything now seems to be running very nicely in production, so it's time write the follow-up I promised. :-) I've written a blog post summarizing my experiences with /// Run a synchronous function `f` in a background worker thread and return its
/// value.
pub(crate) async fn run_sync_fn_in_background<F, T>(
thread_name: String,
f: F,
) -> Result<T>
where
F: (FnOnce() -> Result<T>) + Send + 'static,
T: Send + 'static,
{
// Spawn a worker thread outside our thread pool to do the actual work.
let (sender, receiver) = mpsc::channel(1);
let thr = thread::Builder::new().name(thread_name);
let handle = thr
.spawn(move || {
sender.send(f()).wait().expect(
"should always be able to send results from background thread",
);
})
.context("could not spawn thread")?;
// Wait for our worker to report its results.
let background_result = await!(receiver.into_future());
let result = match background_result {
// The background thread sent an `Ok`.
Ok((Some(Ok(value)), _receiver)) => Ok(value),
// The background thread sent an `Err`.
Ok((Some(Err(err)), _receiver)) => Err(err),
// The background thread exitted without sending anything. This
// shouldn't happen.
Ok((None, _receiver)) => {
unreachable!("background thread did not send any results");
}
// We couldn't read a result from the background thread, probably
// because it panicked.
Err(_) => Err(format_err!("background thread panicked")),
};
// Block until our worker exits. This is a synchronous block in an
// asynchronous task, but the background worker already reported its result,
// so the wait should be short.
handle.join().expect("background worker thread panicked");
result
}I didn't see the bug here. I mean, clearly, the receiver does nothing but block until it gets an answer, so how could it fail? The problem, it turned out, was that the I could try to fix this with controls channels or orderly shutdowns, but it means that my APIs no longer compose as nicely, and that I have to painfully vigilant. I think that these kinds of
Yes, this is normally the behavior that I want. The data on the channel might be, say, the second half of a database row in PostgreSQL
I can think of some use cases where it would be nice to get a
In general, I set
I've written code like this, using things like
As a general rule, most of my programs lean towards functional designs, idempotent designs, or proper transactions. If some data needs to be written to disk, I'm going to design around the fact that I need to see a return value from Except for rendezvous channels, channels have buffers that can contain one or more items. If the receiver has already hit a hard error and given up, what happens to the contents of the channel buffer? Basically, any work in progress inside the sender is essentially just an extension of the channel buffer. If the receiver needs to exit abruptly, then both the channel buffer and the sender's work will be lost. Or the receiver's error handling code will need to carefully shut down all active senders and drain the channel buffers. And error handling code tends to be poorly tested and buggy.
It might be the right choice for |
|
I'm beginning to suspect there's a deeper issue with I wrote up another blog post, because this seemed like it was too long for a GitHub issue. Once again, thank you to everybody for participating in this discussion! I've been learning a lot as we talk about these issues. |
|
I'd like to weight-in against a default unwrapping on each An example use-case is orderly, yet asynchronous, shutdown, where some components might still be running and sending messages to other components which have already, and orderly, shutdown. If you panic on sends in that case, you're going to get a panic 1 out of 2 times when you do a crtl-c and shutdown the whole system. (this actually involves in The reason the result of the send is not handled, is because the component the message is being sent to, in the case of an orderly shutdown, is very likely to shut itself down, hence drop the receiver, before the component that is sending messages to it. How does the sending component shut itself down? Probably in the next iteration of it's event-loop: If the system is shutting itself down in an orderly fashion, any senders corresponding to the receiver of that component will be eventually dropped, and therefore the So the shutdown doesn't depend on the This isn't meant as a generally applicable recipe, just to show that the result of a The case in Servo mentioned above, which required wrapping an earlier version of crossbeam with something that was aware of when the receiver had been dropped, was because there are also use cases when you do want to panic, or otherwise handle the error, when you're trying to send a message to a component that has unexpectedly already dropped it's receiver. In this case, if the receiving component has already shutdown, and a message is still attempted to be send to it, something is wrong hence the The previous crossbeam API which didn't return a The current middle ground, where you can either ignore, or |
|
If I understood everyone correctly, it seems no one strongly believes we should change the signature of Thank you all for the great discussion! |
Hi! This is somewhat of a continuation of "should dropping receiver close the sender" issue. I've migrated rust-analyzer to crossbeam-channel 0.3, and the thing I've noticed is that every
.sendis followed by.unwrap. Perhaps we should make this unwrapping behavior the default, and introduce a separatechecked_sendwhich returns aResult?Sending to a channel without receivers seems to indicate a programmer's error in the majority of cases: there's either a bug in communication structure such that receivers exit too early (direct bug), or a reciver is closed due to a panic (indirect bug). In Rust it is OK to panic on bugs by default, while providing checked alternatives:
[]panic on out of bounds, and has.getfor check accesschecked_addReturning a result by default seems to be bad:
Result<T, SendError>the user has choices:0.2 semanticsEDIT: not exactly, 0.2 blocked), but not optimal: it's better to see a panic than a deadlockAs a counter point, a similar API is exposed by std's mutexes. They return
Results by default, and almost all uses of.lock()are followed by.unwrap/.expect. I would argue that this design is also less ideal, and that std should have panicked by default, providingchecked_lockmethods for rare cases when you do want to handle this somehow.The text was updated successfully, but these errors were encountered: