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

A running subscription without a yield never gets killed if using iced_native::subscription::unfold. #1348

Closed
2 tasks done
davidlenfesty opened this issue May 20, 2022 · 4 comments · Fixed by #1349
Closed
2 tasks done
Labels
bug Something isn't working

Comments

@davidlenfesty
Copy link

Is there an existing issue for this?

  • I have searched the existing issues.

Is this issue related to iced?

  • My hardware is compatible and my graphics drivers are up-to-date.

What happened?

(maybe should be a discussion, maybe should be an issue, I consider this a bug, even if just a documentation issue)

I created a subscription with iced_native::subscription::unfold, and found it to run forever, even when the message indicating it finished got updated and the subscription function of the Application was called. After lengthy investigation, I found that the async function needs to yield at least once after the state is updated to get killed.

What is the expected behavior?

I would have expected that the subscription would be killed once the main application state got updated. There was nothing in the documentation to point me to suspect that I needed to yield to release the subscription and let it be killed.

IMO this definitely should be somewhere in the documentation, it's a very easy issue to hit, and not necessarily obvious. Ideally there should be a yield somewhere in the system (maybe after every iteration or when the returned Message is None?), but am not familiar enough with future streams to offer a good solution.

Version

0.4

Operative System

Linux

Do you have any log output?

No response

@davidlenfesty davidlenfesty added the bug Something isn't working label May 20, 2022
@hecrj
Copy link
Member

hecrj commented May 26, 2022

What do you mean by yielding?

If by "without a yield" you mean never awaiting and returning control to the async runtime, then that would be against the contract of a Future (i.e. blocking in an async context is forbidden):

The core method of future, poll, attempts to resolve the future into a final value. This method does not block if the value is not ready. Instead, the current task is scheduled to be woken up when it’s possible to make further progress by polling again.

@davidlenfesty
Copy link
Author

davidlenfesty commented May 27, 2022

Sorry yes, by yield I do mean awaiting. However I also never block inside my async function, I can even use awaits inside of the main core of my function, but if I neglect to do it when transition to the "finished" state of my stream, now I'm stuck busy looping, when I would have expected returning a command to pull me out of that state. Probably not useful to discuss without a concrete example, so here's pretty much exactly what I've implemented:

(Sorry, I don't have the code in front of me, and went from memory, there might be some errors, but the control flow is the important bit here anyways)

enum UploadState {
    Creating,
    Uploading(f32),
    Finished,
}

// assume this is in my app impl or something
fn subscription(&mut self) -> Subscriotion<Message> {
    // assume the impl sets this when it gets the Message::Finished message
    if self.finished {
        return Subscription::none();
    }
    subscription::unfold(UploadState::Creating, async |state| {
        match state {
            UploadState::Creating => (Some(Message::Uploading(0.)), UploadState::Uploading(0.)),
            UploadState::Downloading(progress) => {
                if (progress < 100.) {
                    progress += 1.;
                    tokio::sleep(Duration::from_millis(100);
                    (Some(Message::Uploading(progress)), UploadState::Uploading(progress))
                } else {
                    (Some(Message::Uploading(100.)), UploadState::Finished)
                }
           }
            // This will now busy loop forever, unless I put an await in this match arm
            UploadState::Finished => (Some(Message::Finished), UploadState::Finished)
         }
    })
}

A naive expectation would be that if a message is returned that would return control to the runtime somehow, after all a message should get sent out, but it doesn't, and the message gets queued until I put an await inside the final match arm.

I don't think this is good behaviour because it means you have to yield arbitrarily to get a message out, even if your closure does no sync blocking.

@hecrj
Copy link
Member

hecrj commented May 27, 2022

A Subscription represents an asynchronous stream of messages. Because of its concurrent nature, there are no guarantees in the order of operations between a subscription and the iced runtime.

In other words, when a Subscription produces a Message there is no guarantee update will be called immediately. A Subscription runs on its own and the application processes its messages asynchronously.

Therefore, your Subscription is incorrectly implemented because it assumes update and subscription will be called right after the Finished message. This assumption is incorrect, and an infinite loop happens as a result.

However, the application should eventually process the Finished message and kill the infinite loop, which does not happen... This is the bug that needs fixing! I have opened #1349, which should fix this issue.

But keep in mind that your Subscription is still incorrect and will not work as you expect it to. Even with the fix, multiple Finished messages may be produced by the Subscription before it is finally canceled.

@davidlenfesty
Copy link
Author

Perfect! Thank you for looking into this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants