Using a 0 sized buffer causes broadcasts to never be received

[MJDSys]

When a bus is created with a buffer of 0, messages are successfully broadcast, but are never received. I'm not sure if you care about such a use case (I want to receive only messages being sent now, not messages that may have been sitting in the queue waiting for a reader), but if not it woudl be good to get a message/panic when creating such a bus. Code:

extern crate bus;

use bus::Bus;
use std::thread;
use std::time::Duration;
fn main() {
    let mut bus = Bus::new(0);
    let mut rx = bus.add_rx();
    let t = thread::spawn(move || {
        rx.recv().unwrap();
    });
    thread::sleep(Duration::from_millis(500));
    println!("Sending");
    bus.broadcast(());
    println!("Sent, waiting on receive");
    t.join().unwrap();
    println!("Received and joined");
}

In this minimal example, the sent message is always printed, but the final received/joined message is not. Increasing the buffer to 1 makes it complete successfully.

[MJDSys]

After reading the sources for this crate and thinking some more, it seems the 0 length case isn't useful. In my case, I only create receivers once they are needed, so they can only receive messages after that when they are listening. Thus I think the 0 length case should be considered an error and panic!ed, with some documentation that length needs to be >=1. Does that make sense? Or should the 0 length case be handled anyways, as a synchronisation mechanism between the producer and consumers?

[jonhoo]

Hmm, I think the 0-length case degenerates entirely, because the head and tail will always point to the same value, which I'm sure triggers some weird infinite loops or something. I'd have to read the code again carefully to figure out exactly what happens. I'm perfectly happy to require the length be >= 1 (the code already adds one to the length to make room for the fence). It's not entirely clear to me what the best failure communication mechanism is though -- an assert!() seems aggressive. One alternative would be to simply check if the argument is 0, and if so, make it 1... Thoughts?

[MJDSys]

My only thought on making 0 be 1 is that people who expect a 0 reader case with no buffer would expect the sender to block. That's why I suggested assert!, to make it clear that they shouldn't do that. I think a documentation update with any option will be fine. Maybe the best move is to make 0 into a 1 and during debug builds print a warning? That would avoid the aggressiveness, while alerting new users?

I think what made me originally try using 0 was that I didn't realize new receiver wouldn't receive previous broadcasts. I see you mentioned that in the documentation, but maybe it needs to be yelled louder so it gets through thick skulls like mine? If I can figure out a nice way to do that, would a PR be welcome?

[jonhoo]

Absolutely! Something along those lines I'd be happy to merge.

[jonhoo]

@MJDSys still interested in writing up a PR for this?

[MJDSys]

@jonhoo I still basically am, I just got busy on other projects that don't (potentially yet!) need this library. I'll try to swing back to this issue and submit that PR, but no guarantees.

Thanks for this library though! It was great to use.

[jonhoo]

Glad to hear you found it useful! Hopefully the API and documentation was also understandable?

[MJDSys]

Yep, besides from this one issue everything was great. You can see my use of this crate here: https://gitlab.mjdsystems.ca/MJDSys/reload-rs/blob/master/src/watcher.rs . I use it to notify several watchers when a set of files have modified, while only having one actual set of filesystem watchers.

[jonhoo]

Looking at that code, I feel like you'd be better off creating a Receiver for every watcher, instead of giving them an Arc<Mutex<Sender>> that they then have to lock each time to get a single-use receiver...

[MJDSys]

Do you mean the Arc<Mutex<Bus>>? I wasn't able to pre-allocate the Receivers, as I didn't know how many Handlers there would be. Considering the use case (local development server for developer use only, not exposed to the internet), the overhead of the mutex was an acceptable compromise.

The Sender is just to block the watcher thread, so if no one is listening for events I don't try to broadcast them. There is a potential race in there I was trying to avoid as well, though nothing unsafe.

Is there any easy way to make receivers for a bus on demand without a lock? Or would you just have to preallocate them?

[jonhoo]

I think my recommendation would be to have each Handler have an Option<Receiver> as well as the Arc<Mutex<Bus>>. If their Option is None, then they get a new one from the Mutex and then put it in the Option. Next time that Handler can then just use the Receiver directly.

[MJDSys]

Yeah, I was thinking about some sort of cache like that. If I ever swing back to that project and rework that file, I'd probably try that. And I'd definitely do that for a more "real world" project.

Regardless, for what I needed out of that code it works great, and it saved me a ton of time to just use this crate. I'd definitely consider using it again if any future project requires it!

[64kramsystem]

an assert!() seems aggressive. One alternative would be to simply check if the argument is 0, and if so, make it 1... Thoughts?

I actually think that raising an error is the only way to go 🙂

Making code robust against unintended state is a solid programming principle, however, initialization of the Bus struct (Bus::new(0);) is something intended by the developer, so the approach should be different. Essentially, the Bus initialization value is a configuration, and I think failfast behavior is desirable, in order to detect misconfiguration (that is, initialization with a 0 value).