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

Enable multi_future::get for void multi futures #62

Closed
wants to merge 1 commit into from

Conversation

PeterTh
Copy link

@PeterTh PeterTh commented Jul 26, 2022

Describe the changes

This PR adds a specialization of multi_future::get which allows using it on void multi-futures. Without this change, as far as I can tell, it's impossible to catch exceptions thrown by loop bodies in the common parallelize_loop case where the body does not return a value. In fact, any such exceptions get silently consumed and the execution of that parallel part of the loop is stopped. That's a pretty significant usability issue, and can be initially difficult to debug.
With this PR, the user can at least call get on the multi-future to ensure that an exception is forwarded.

Testing

We tested this in our code base. Of course it would be easy to add tests for it in the test program as well, but I didn't want to invest that effort before knowing whether the PR would be accepted.

Additional information

Another option would be to change the existing wait to use get. That would reduce the foot-gun potential, but would not be as close to the std::future API. On the other hand, I'm not sure if the "just silently ignore exceptions" use case really needs to be supported.

@bshoshany
Copy link
Owner

Thanks for the pull request, @PeterTh! This is indeed something I overlooked. I will do some testing and then incorporate this into the next version of the library, which is planned to be released in a few days.

However, instead of std::enable_if_t I might implement this with C++17 if constexpr, since I personally find it to be more elegant :)

@bshoshany bshoshany closed this Jul 27, 2022
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

Successfully merging this pull request may close these issues.

2 participants