antoyo / relm

Idiomatic, GTK+-based, GUI library, inspired by Elm, written in Rust
MIT License
2.44k stars 78 forks source link

EventStream panics on emit from another thread #278

Open antoyo opened 3 years ago

antoyo commented 3 years ago

This is probably not what we want.

There should be a way to check if the stream is closed.

Schmiddiii commented 3 years ago

To clarify that issue, emitting on another thread will not always panic. The panic occurs when sending a message to a widget that was dropped during the calculation of the second thread.

Consider for example a modified multithread-example, where the calculation label is a saparate widget. The main window contains this label and a button that removes the label from the window when clicked. When clicking the button during the calculation, the label widget will be dropped and its EventStream closed. But after the calculation finishes, a message will be sent using the EventStream, which was closed. This will lead to the panic Trying to call emit() on a dropped EventStream right here.

Creating a method to check if the stream is closed can lead to a race condition, so it is no real solution:

I would have a few suggestions on how to fix this issue, but each has some problems:

Solution 1

Just remove the else-clause with the panic!. I see no reason why someone would want to check if the message was sent.

Problems: Maybe someone might want to know if the message was sent (maybe for logging).

Solution 2

Make emit return Result<(), Error>, Ok(()) when the EventStream was open and Err(Error) if it was closed.

Problems: Will break compatibility to earlier versions. It would probably be tedious to write .unwrap_or(()) after each emit.

Solution 3

Make emit never panic (ignore the request if EventStream was closed) and create a new function emit_checked(&self, msg::Msg) -> Result<(), Error> similar to solution 2.

Problems: As emit, lock and observe all panic if the EventStream was closed, this method would need to be repeaded three times. Furthermore the codebase would now have two functions doing the same thing just having a different return type.

Solution 4

Make some function returning a reference to the EventStream of the StreamHandle (maybe using a callback).

Problem: This will make set_callback usable, that may break the application when overwriting the main callback (I'm not sure what the consequences will be as I did not develop the library and only took a look at the code for a few minutes).

Result

Every single solution has some problems. Furthermore, the first three solutions have a problem with lock. As lock has a return type, it would have to return a Lock even if the EventStream was closed. This could be resolved by returning a Result, that would break compatibility, or having a Lock locking nothing.

Other solutions are obviously welcome.