asynchronics / asynchronix

High-performance asynchronous computation framework for system simulation
Apache License 2.0
178 stars 10 forks source link

Panic when trying to iterate over empty EventStream #36

Closed jajetloh closed 2 months ago

jajetloh commented 2 months ago

I get the following error log

thread 'Worker #0' panicked at C:\   ...   \index.crates.io-6f17d22bba15001f\asynchronix-0.2.2\src\model\ports.rs:121:53:
called `Result::unwrap()` on an `Err` value: BroadcastError
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
error: process didn't exit successfully: `target\debug\process-simulation-runner.exe` (exit code: 101)

Using the following minimal example

[dependencies]
asynchronix = "0.2.2"
use asynchronix::model::{Model, Output};
use asynchronix::simulation::{Mailbox, SimInit};
use asynchronix::time::MonotonicTime;

#[derive(Default, Debug)]
struct Source {
    output: Output<f64>,
}
impl Model for Source {}
impl Source {
    async fn send(&mut self, value: f64) {
        self.output.send(value).await;
    }
}

fn main() {
    let mut source = Source::default();
    let source_mb: Mailbox<Source> = Mailbox::new();
    let source_address = source_mb.address();
    let source_stream = source.output.connect_stream();

    let mut sim = SimInit::new()
        .add_model(source, source_mb)
        .init(MonotonicTime::EPOCH);

    source_stream.0.for_each(|x| println!("From stream: {:?}", x)); // Attempt to iterate over stream
    sim.send_event(Source::send, 1.234, &source_address);  // Send event through output port
}

In the second-last line, I attempt to iterate over the stream. Expected behaviour would be nothing as I imagine the iterator would be empty, but instead the code panics. Or at least some explanation in the error handling of the cause. However if the second-last and last lines are swapped (so the event is sent first then we iterate over the stream), no panic is thrown, and everything works as expected.

Any chance to get this edge case looked at? Not urgent for my own use case, but I imagine other users will run into this (if they haven't already)

sbarral commented 2 months ago

Thank you for the report! This was surprising to me too, but upon inspection I see that this was actually intended behavior :-) Now I admit that your example makes me question the rationale for this decision...

What happens here is that source_stream is consumed by for_each, so there is no more stream to send the value to (note that the panic actually occurs when sending on the last line, not when iterating). If for instance the stream is made mutable and for_each is substituted by by_ref().for_each(), then no panic is thrown.

But I agree, this behavior is too surprising and there may be legitimate reasons for dropping the stream while the simulation is still running. I am busy with urgent matters this week but I definitely want to revisit this topic, so let's keep this issue open and please don't hesitate to bump this thread in another 7 days if nothing happens.

sbarral commented 2 months ago

In fact I see that this behavior is "fixed" on the main branch because EventBuffer, which replace EventStream in the development version, simply ignores attempts to send to a dead endpoint without panicking. What's more, EventSlot does the same in both v0.2.2 and in the development version, so having EventStream panic in this situation is at the very least inconsistent. Since this behavior no longer occurs on the main branch I guess we could close the issue, but if you need a 0.2.3 interim release please let me know.

jajetloh commented 2 months ago

Fantastic to hear it's fixed in 0.2.3, thanks for the super prompt response! Happy for this issue to be closed then, and happy to wait for the official 0.2.3 release.

sbarral commented 2 months ago

Oh, sorry for the misunderstanding: what is in main will end up in v0.3.0 which has been "soon to be released" for quite some time now ;-) So what I meant is that if you need it now rather than in a couple of months I can make a minor v0.2.3 release with just this fix.

jajetloh commented 2 months ago

Ahh in that case, the 0.2.3 release sooner with the fix would be fantastic, as long as it's not too much work on your end!

sbarral commented 2 months ago

Fixed in 0.2.3 :tada: