Skip to content

Conversation

@MitalAshok
Copy link
Contributor

@MitalAshok MitalAshok commented Feb 15, 2022

A fix for #2488 without breaking ABI compatibility.

For types that are not default constructible, promise<T> will now act like it was supposed to all along (set_value will copy/move construct a value rather than copy/move assign it). The value is stored like a std::optional, where _Has_stored_result is repurposed to be "has a value been constructed", and its old purpose is replaced with _Has_stored_result || (_Exception != nullptr).

Differences are guarded with is_default_constructible<_Ty> to have the same behaviour as before, except that promise<T>::set_exception(nullptr) has been explicitly disallowed (It was already UB, since a precondition of set_exception_ptr/set_exception_at_thread_exit is "p is not null.", and would make get return a default constructed value).

I also removed a member function void _Set_value(bool _At_thread_exit); that had the comment // store a (void) result and its corresponding _Set_value_raw function, since they were unused and it would be a logical error to call them with a non-default-constructible type.

In vNext, promise<T> can be fixed for all types by replacing all the is_default_constructible<_Ty> checks with false.

@MitalAshok MitalAshok requested a review from a team as a code owner February 15, 2022 00:25
@ghost
Copy link

ghost commented Feb 15, 2022

CLA assistant check
All CLA requirements met.

@MitalAshok MitalAshok force-pushed the promise-not-default-constr branch 2 times, most recently from f90ef17 to 042e51f Compare February 15, 2022 00:34
@CaseyCarter CaseyCarter added the bug Something isn't working label Feb 15, 2022
@CaseyCarter CaseyCarter linked an issue Feb 15, 2022 that may be closed by this pull request
@MitalAshok MitalAshok force-pushed the promise-not-default-constr branch from 042e51f to 47e5e09 Compare February 15, 2022 00:39
Copy link
Member

@StephanTLavavej StephanTLavavej left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this long-standing bug! (I can't count how many times it's been reported over the years, but it's definitely "lots".) Main feedback here is a request for test coverage, plus minor codebase convention nitpicks. Your strategy looks good to me and it should preserve binary compatibility.

@MitalAshok
Copy link
Contributor Author

I've also (re)discovered this bug:

STL/stl/inc/future

Lines 265 to 275 in afe0800

virtual _Ty& _Get_value(bool _Get_only_once) {
unique_lock<mutex> _Lock(_Mtx);
if (_Get_only_once && _Retrieved) {
_Throw_future_error(make_error_code(future_errc::future_already_retrieved));
}
if (_Exception) {
_Rethrow_future_exception(_Exception);
}
_Retrieved = true;

Also seen here: https://stackoverflow.com/a/42648634

Since _Retrieved is not set before the exception is thrown, the shared state isn't invalidated when future::get throws an exception, and the post condition [futures.unique.future]p17 "Postconditions: valid() == false" isn't held.

The fix would be a breaking change. I could have it fixed only with the new behaviour, or have the new behaviour follow the old behaviour in this edge case and just add a comment that it needs to be fixed.

@StephanTLavavej

This comment was marked as resolved.

Copy link
Member

@StephanTLavavej StephanTLavavej left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test looks good, thank you! One more set of small comments (the most significant being the Standard citation, which isn't very significant) and I think this will be ready to go 🚀

@StephanTLavavej
Copy link
Member

I've pushed a merge with main (no conflicts) followed by Casey's requested changes (I added backticks for consistency to one comment, no other changes). Should be ready to merge after @CaseyCarter's swift approval.

@CaseyCarter CaseyCarter removed their assignment Mar 2, 2022
@StephanTLavavej StephanTLavavej self-assigned this Mar 3, 2022
@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit 38ddb14 into microsoft:main Mar 4, 2022
@StephanTLavavej
Copy link
Member

Thanks again for fixing this long-standing bug, and congratulations on your first microsoft/STL commit! 🎉 🚀 😻

This will ship in VS 2022 17.2 Preview 3.

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 this pull request may close these issues.

<future>: promise default constructs the value type

4 participants