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

Lifetime of receivers returned by operator | #248

Closed
sdebionne opened this issue Mar 27, 2019 · 8 comments
Closed

Lifetime of receivers returned by operator | #248

sdebionne opened this issue Mar 27, 2019 · 8 comments

Comments

@sdebionne
Copy link
Contributor

The title is probably not very explicit, hopefully the following snippet explains a bit better:

auto [send, receive] = stlab::channel<int>(stlab::default_executor);

std::atomic_bool done{ false };
receive | [&_done = done](lima::any_image_view_t) {
    std::cout << "Done!" << std::endl;
    _done = true;
};

receive.set_ready();
send(1);

// Waiting forever the end of the processing
while (!done.load()) {
    std::this_thread::sleep_for(std::chrono::milliseconds(1));
}

This code is looping forever, because the process that set done is never called. The reason is a missing auto hold = in:

auto hold = receive | [&_done = done](lima::any_image_view_t) {
    std::cout << "Done!" << std::endl;
    _done = true;
};

It took me a while to figure that out. Shouldn't the channel take ownership of the intermediate receivers or something like that? It's probably not possible since they may have different types... I wish an error would be triggered for this kind of coding mistake.

@vdeurzen
Copy link

The problem is that you need to have a reference to the full processing graph somewhere. If you do not assign the result of operator| (also for other operators) to a variable it is essentially constructed and immediately destructed due to not being used. This is similar to the creating an unnamed lock_guard, it will construct, and immediately destruct unless you keep a handle around within the scope you use it in.

In C++17 we could add the [[nodiscard]] attribute to encourage the compiler to warn the result is unused. @FelixPetriconi would this be an option?

@FelixPetriconi
Copy link
Member

I run into the problem at the beginning as well. But after I stepped once into this trap, it never happened to me again.
I think [[nodiscard]] is a good idea. And we should consider this for the futures as well. The channel cannot take the ownership of the intermediate receivers, because this would prevent cancellation, a central feature of futures and channels.

@aaronalbers
Copy link
Contributor

@FelixPetriconi Is it possible to add a .detach() function to receivers? That way you don't have to find a place to store the leaf receiver if you just want it to live until the upstream sender is closed.

@FelixPetriconi
Copy link
Member

I started looking into this, but it is not as simple as for the futures, because currently one cannot attach a so called process (e.g. a lambda) with no arguments to a receiver. So receiver<void>{} | []{} is not possible. So this problem would have to be solved before, then a detach is a two liner. I will put this on my list.
Many thanks for the suggestion.

@aaronalbers
Copy link
Contributor

@FelixPetriconi It might also be worth thinking about adding a .then() to receivers as well. It could signify the function to call when the channel closes. One major use case would be to store tokens required by the graph until they are no longer needed. Possibly even a .recover() for when the channel closes via an error.

@aaronalbers
Copy link
Contributor

It might also be worth making some of the channel interfaces more like the future interfaces which can handle ranges.

template <typename M, typename S, typename F, typename... R>
auto merge_channel(S s, F f, R... upstream_receiver) {}

should have another interface like

template <typename M, typename S, typename F, typename I>
auto merge_channel(S s, F f, std::pair<I, I> range) {}

same is true for zip_with and zip

@FelixPetriconi
Copy link
Member

You are true, this is missing. New issue created: #257

@FelixPetriconi
Copy link
Member

Fixed in 1.5.0

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

No branches or pull requests

4 participants