Channel #101
Channel #101
Conversation
|
In my view one of the nastiest classes of bugs in asynchronous systems is
missed notifications.
Take a condition variable as an example:
Let's say fiber A .waits() on it, and then fiber B notify() it. In 99.9% of
the cases that happen in this order and you are happy.
Then due to one small timing issue the notify() happens before the wait(),
you miss the notification, and good luck figuring out why.
(This is part of the reason I don't offer a condvar in Scipio, btw. I
essentially haven't figured out a way to make it immune to that without
essentially making it into a gate)
For some usages of a channel, a silent success over a closed channel would
eat up a notification to a similar effect.
Say you have a fiber that is generating data, and another one that is
responsible for writing it to disk. You send a request for flush/fsync over
the channel,
and that gets silently dropped because the receiver is gone. Granted, it's
not trivial to decide what to do, which is why I picked this example. But
it's certainly
better than not ever knowing this happens when you are trying to debug what
becomes essentially a rare data corruption on crash.
So given the above I do not agree that a send over a closed channel should
ever be silent.
To be clear, I do agree with the reasoning you made but a case like this is
a case of essentially like picking a politician for office.
Deep down you know they are all bad, you just have to choose which aligns
more with the set of issues that you care about.
…On Fri, Oct 9, 2020 at 4:45 AM Aleksey Kladov ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In scipio/src/channels/local_channel.rs
<#101 (comment)>:
> + ///
+ /// # Examples
+ /// ```
+ /// use scipio::{LocalExecutor, Local};
+ /// use scipio::channels::LocalChannel;
+ ///
+ /// let ex = LocalExecutor::make_default();
+ /// ex.run(async move {
+ /// let (sender, receiver) = LocalChannel::new_bounded(1);
+ /// sender.send_eventually(0).await.unwrap();
+ /// drop(receiver);
+ /// });
+ /// ```
+ ///
+ /// [`send`]: struct.LocalSender.html#method.send
+ pub async fn send_eventually(&self, item: T) -> io::Result<()> {
Additional point of interest: there was a long discussion about the result
of send. There are a regiments for the position that send should never
fail, and that sending over a closed channel should be an error:
crossbeam-rs/crossbeam#314
<crossbeam-rs/crossbeam#314>
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#101 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACQ3PJ7P2THUXO2TAI5MHLSJ3ETNANCNFSM4SJNQZLQ>
.
|
+100500, I don't really think it's productive to debate this, I just wanted to give some extra context here. On thing I want to clarify though, is that the alternative proposal is panicking on send if the receiver is gone. I wouldn't call this silent; on the contrary, it elevates recoverable
Not sure if this fully covers all the footguns, but I quite like how std does notification's API. Condvar's wait consumes a mutex guard, which prevents some logical races. Thread parking API just makes |
|
Hey, not all discussions have to be productive! For some we can settle on fun =) Panic vs requiring |
glommer
force-pushed
the
channel
branch
2 times, most recently
from
91d5e15 to
55c2455
Compare
|
I just added a second commit implementing most suggestions from @matklad to make review easier. I will stash it into a single commit once we're all in agreement In particular:
As we have discussed:
|
scipio/src/channels/local_channel.rs
Outdated
| send_waiters: Vec::new(), | ||
| recv_waiters: Vec::new(), | ||
| receiver_alive: true, | ||
| sender_alive: true, |
There was a problem hiding this comment.
I wonder if it makes sense to pair waiters: Vec and alive: bool into a single waiters: Option<Vec>?
That way, type system guarantees that you can‘t add waiter if the opposite side is closed.
|
You'll be looking for alternative to `join_all` fom futures. Here it is:
smol-rs/futures-lite#2 (comment)
…On Wed, 14 Oct 2020 at 16:12, Glauber Costa ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In scipio/src/channels/local_channel.rs
<#101 (comment)>:
> @@ -0,0 +1,622 @@
+// Unless explicitly stated otherwise all files in this repository are licensed under the
+// MIT/Apache-2.0 License, at your convenience
+//
+// This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2020 Datadog, Inc.
+//
+use crate::channels::ChannelCapacity;
+use futures::Stream;
futures-lite it is.
A lot of scipio itself was inspired by his work so I am totally cool with
that.
So we don't need to list futures-core as a crate right ?(right now it is
not).
I'll just go ahead later today and delete futures from the cargo list.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#101 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AANB3M3LX4ETVPUZFKY23ZTSKWWT3ANCNFSM4SJNQZLQ>
.
|
|
My only drive-by comment is that it would be nice to have a Though, I would also be more than open to alternative monitoring strategies. |
|
I have pushed a new version that incorporates both of your suggestions (@matklad and @Daniel-B-Smith) Thank you both. However for tests I still can't use But @matklad if you could advise what would be needed to get those tests working with futures-lite, that I would be quite enlightening |
|
I managed to convert most tests to futures_lite by using Now, the problem lies on |
|
... which must be some old bug! It works if I update |
|
Everything is using futures-lite now with the exception of the one test in which we test that For this to compile, though ,we need PR #115 merged first as it bumps the version of futures-lite (while removing futures from Cargo) |
Channels are useful abstraction when data needs to be passed between
asynchronous entities.
I have just came across a use case where channels would provide a much
more ergonomic way to code than the Deque, so here's our first!
The LocalChannel is an executor-local channel (meaning !Send, !Sync)
that is useful to pass data between task queues.
The use case for this is an internal service that needs to pick up
work to do serially. Imagine for example a flush service that wants
to flush one file at a time.
The flushers will live in a separate task queue. The entities generating
the work now have to register it into the flusher's task queue.
Using a Deque is possible but you now have to wrap it under an Rc
and code a loop-like construct (and hey, if that's what float your
boat, have at it!)
Using the LocalChannel, however, we can write this:
let (sender, receiver) = LocalChannel::new_unbounded();
Local::local_into(async move {
receiver.for_each(|x| { do_something(x) }).await;
}, tq).unwrap().detach();
sender.send(...);
|
pushed a new version that should fix all comments here, plus the ones raised in #111 (which included a copy of this, and some people commented there)
|
There was a problem hiding this comment.
LGTM!
Couple of "what if" alternatives:
-
I wonder if we can remove
dequemodule after merging this? It feels a bit confusing to me: it is in collections, but is actually a synchronisation mechanism. I think people just provide channel interface instead of deques in rust typically. -
for the code organization I personally have a mildly-strong preference to putting all
pubthings on top: https://github.com/rust-analyzer/rust-analyzer/blob/0c67edc0f7ebb7e6a265d4a925bc368e460cd8cb/docs/dev/style.md#order-of-items. I often just read source code as documentation (issuing "go to definiion" in the editor is faster than opening the browser), so I like when the prefix of file can be read as API docs.
|
The Deque can go. A bit of history, In our Scipio-based internal application I was using it to implement a write-behind / read-ahead mechanism similar to what we now have in the It is still not the same as the channel, because technically you could implement urgency into the Speaking of enhancing the channel, I am considering implementing a bidirectional channel too (should be easy with just two channels playing the role of both lanes). If you look at the controller code I am doing that manually which may mean there is room for such abstraction. Thoughts? About the code organization: as I said before already while our community is small we can have those discussions but I do hope it grows! When it does, programmers tend to naturally bite each other too much about style and I always hated that. So I found it very refreshing that Rust has clippy and rustfmt which allowed me to essentially come with the the "if they don't complain, everything goes" policy. That means I will certainly not oppose it if you do it this way but I'd be wary of enforcing such a rule. Now in my personal opinion, it does sound like a good rule. I am wondering if we couldn't get Clippy to shout about it ? |
Agree, the only rule we should enforce is "CI is green". If some guideline can't be automatically checked, it's futile to enforce it. Though, having non-enforceable/non-enforced guidelines is still useful to steer new code and refactorings in the right direction.
I don't think so. Well, in theory we can contribute a lint for that, but I wouldn't want to use this -- this is a pretty nuanced guideline, and I fear would have a fair amount of false positives if enforced by a |
|
Hey, robots have feelings too. |
Depends on #100.
Will merge that first.