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

Propagate abandoned future errors via future<T>::then() #13314

Closed
coryan opened this issue Dec 15, 2023 · 0 comments · Fixed by #13316
Closed

Propagate abandoned future errors via future<T>::then() #13314

coryan opened this issue Dec 15, 2023 · 0 comments · Fixed by #13316
Labels
priority: p3 Desirable enhancement or fix. May not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@coryan
Copy link
Contributor

coryan commented Dec 15, 2023

Consider this code:

std::future<int> F() { return std::promise<int>().get_future(); }
google::cloud::future<int> G() { return google::cloud::promise<int>().get_future(); }

The expression F().get() will throw a std::future_error(std::future_errc::broken_promise)1:

https://en.cppreference.com/w/cpp/thread/promise/%7Epromise

The expression G().get() behaves the same way, that all sounds good. Now consider this function:

google::cloud::future<int> H() {
  return google::cloud::promise<int>().get_future().then([](auto g) { return 2 * g.get(); });
}

Currently H().get() deadlocks. That is not a good thing. I can fix it, the fix is trivial, but unfortunately we have unit tests that depend on the existing behavior.

We should fix the tests, unfortunately it is not that easy. These tests (and the code they are testing) needs to work with exceptions disabled (i.e., the noex builds aka -fno-exceptions). With exceptions disabled .get() terminates the program if the promise was abandoned (or the future<T> contains any exception).

So we need to find what promise is being destroyed and abandoning the future, and most likely, set the value to some kind of google::cloud::Status before destroying the promise.

Footnotes

  1. this is the only sensible choice. Once cannot leave .get() blocked forever, and there is no valid int value to return.

@coryan coryan added type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. priority: p3 Desirable enhancement or fix. May not be included in next release. labels Dec 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p3 Desirable enhancement or fix. May not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant