Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EventStream panics on emit from another thread #278

Open
antoyo opened this issue Mar 14, 2021 · 1 comment
Open

EventStream panics on emit from another thread #278

antoyo opened this issue Mar 14, 2021 · 1 comment

Comments

@antoyo
Copy link
Owner

antoyo commented Mar 14, 2021

This is probably not what we want.

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

@Schmiddiii
Copy link
Contributor

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:

  • Check if EventStream is closed. It is not.
  • EventStream closed by another thread.
  • Sending a message using the EventStream. This will still lead to the panic

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants