microsoft / microsoft/STL

<future>: incorrectly used copy assignment instead of copy construction in set_value

Open
#1,190 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug vNext
Dominant language
C++
Stars
11.1k
Forks
1.7k
Avg merge
4d 15h
Merged PRs (30d)
22

Description

Describe the bug
Apparently promise::set_value should copy construct or move construct instead of assigning.

[futures.promise]/17:

Throws:

(17.1) future_­error if its shared state already has a stored value or exception, or
(17.2) for the first version, any exception thrown by the constructor selected to copy an object of R, or
(17.3) for the second version, any exception thrown by the constructor selected to move an object of R.

(unfortunately not found more direct clues that it should construct, not assign)

Command-line test case

D:\temp2>type simplified_set_value_const.pass.cpp
#include <future>
#include <cassert>

struct A
{
    A() {}
    A(const A&) {
        throw 10;
    }
};

int main(int, char**)
{

        typedef A T;
        T i;
        std::promise<T> p;
        std::future<T> f = p.get_future();
        try
        {
            p.set_value(i);
            assert(false);
        }
        catch (int j)
        {
            assert(j == 10);
        }

        return 0;
}

D:\temp2>cl /EHsc  simplified_set_value_const.pass.cpp
Microsoft (R) C/C++ Optimizing Compiler Version 19.28.29115 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

simplified_set_value_const.pass.cpp
Microsoft (R) Incremental Linker Version 14.28.29115.0
Copyright (C) Microsoft Corporation.  All rights reserved.

/out:simplified_set_value_const.pass.exe
simplified_set_value_const.pass.obj

D:\temp2>simplified_set_value_const.pass.exe
Assertion failed: false, file simplified_set_value_const.pass.cpp, line 22

Expected behavior
Exception is thrown and caught, so the program does not trigger assert.

STL version
https://github.com/microsoft/STL/commit/f3ca37d67c4bf18938497766a86aa78188b3ba3b

Additional context
Skipped test:
https://github.com/microsoft/STL/blob/06827feb4cdc4d2328dfbfab9fd5302de6058dd9/tests/libcxx/expected_results.txt#L637-L638

vNext note: Resolving this issue will require breaking binary compatibility. We won't be able to accept pull requests for this issue until the vNext branch is available. See #169 for more information.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start at the C++ standard's futures.promise requirements and the promise::set_value implementation, then reproduce the behavior with simplified_set_value_const.pass.cpp. Check the skipped libcxx result in tests/libcxx/expected_results.txt; done means the copy overload uses construction semantics and the exception is thrown and caught, subject to the vNext binary-compatibility constraint.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
backend, testing-qa
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
30/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.