Skip to content

Allow promises to be constructed for non-default-constructible types#2568

Merged
StephanTLavavej merged 8 commits intomicrosoft:mainfrom
MitalAshok:promise-not-default-constr
Mar 4, 2022
Merged

Allow promises to be constructed for non-default-constructible types#2568
StephanTLavavej merged 8 commits intomicrosoft:mainfrom
MitalAshok:promise-not-default-constr

Conversation

@MitalAshok
Copy link
Contributor

@MitalAshok MitalAshok commented Feb 15, 2022 •
edited
Loading

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 •
edited by ghost
Loading

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
<future>: promise default constructs the value type #2488
Open
@MitalAshok MitalAshok force-pushed the promise-not-default-constr branch from 042e51f to 47e5e09 Compare February 15, 2022 00:39
frederick-vs-ja
frederick-vs-ja reviewed Feb 15, 2022
View reviewed changes
frederick-vs-ja
frederick-vs-ja reviewed Feb 15, 2022
View reviewed changes
frederick-vs-ja
frederick-vs-ja reviewed Feb 15, 2022
View reviewed changes
StephanTLavavej
StephanTLavavej requested changes Feb 16, 2022
View reviewed changes
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

MitalAshok commented Feb 16, 2022

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.

Sign in to view
StephanTLavavej
StephanTLavavej requested changes Feb 17, 2022
View reviewed changes
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 🚀

@fsb4000 fsb4000 mentioned this pull request Feb 18, 2022
Merged
@CaseyCarter CaseyCarter self-assigned this Feb 23, 2022
CaseyCarter
CaseyCarter suggested changes Feb 24, 2022
View reviewed changes
@StephanTLavavej StephanTLavavej mentioned this pull request Mar 1, 2022
Open
@StephanTLavavej
Copy link
Member

StephanTLavavej commented Mar 1, 2022

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

StephanTLavavej commented Mar 3, 2022

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

StephanTLavavej commented Mar 4, 2022

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.

@invertego invertego mentioned this pull request May 4, 2022
Closed
@invertego invertego mentioned this pull request Jun 4, 2022
Open
This was referenced Sep 10, 2024
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@StephanTLavavej StephanTLavavej StephanTLavavej approved these changes

+2 more reviewers

@frederick-vs-ja frederick-vs-ja frederick-vs-ja left review comments

@CaseyCarter CaseyCarter CaseyCarter approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@StephanTLavavej StephanTLavavej

Labels

bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

<future>: promise default constructs the value type

4 participants

@MitalAshok @StephanTLavavej @CaseyCarter @frederick-vs-ja