packaged_task doesn't correctly handle moving results

[onlyone@…]

I want to create a packaged task which wraps a function which returns an object which is movable but non-copyable. I tried:

#include <boost/thread.hpp>
struct MovableButNonCopyable {
    MovableButNonCopyable() = default;
    MovableButNonCopyable(MovableButNonCopyable const&) = delete;
    MovableButNonCopyable& operator=(MovableButNonCopyable const&) = delete;
    MovableButNonCopyable(MovableButNonCopyable&&) = default;
    MovableButNonCopyable& operator=(MovableButNonCopyable&&) = default;
};
int main()
{
    boost::packaged_task<MovableButNonCopyable>([]{return MovableButNonCopyable();});
}

This does not compile, because the instantiation of packaged_task results in the generation of a function that attempts to call the deleted copy constructor for MovableButNonCopyable.

I have determined that this is due to a bug at future.hpp:325, where an rvalue-to-lvalue decay is allowed to occur, which results in the incorrect function overload being used. A patch is attached which adds the required cast.

[onlyone@…]

Fix for #6174

[viboes]

Please, could you stay with which compiler this doesn't compile and log the compile error?

I have no access to lambdas now. The following compiles for me with clang

    boost::packaged_task<MovableButNonCopyable>(MovableButNonCopyable());

[onlyone@…]

Replying to viboes:

Please, could you stay with which compiler this doesn't compile and log the compile error?

I have no access to lambdas now. The following compiles for me with clang

    boost::packaged_task<MovableButNonCopyable>(MovableButNonCopyable());

Bizarre. I tried the following:

#include <boost/thread.hpp>
struct MovableButNonCopyable {
    MovableButNonCopyable(){}
    MovableButNonCopyable(MovableButNonCopyable&&){}
    MovableButNonCopyable& operator=(MovableButNonCopyable&&){return *this;}
private:
    MovableButNonCopyable(MovableButNonCopyable const&);
    MovableButNonCopyable& operator=(MovableButNonCopyable const&);
};

MovableButNonCopyable construct() {return MovableButNonCopyable();} 

int main()
{
    boost::packaged_task<MovableButNonCopyable>(construct);
}

And it compiles without issue with both Clang and GCC. However, the code that I presented in the original ticket fails under GCC 4.6.2 (Macports version, x86_64), with the following message:

In file included from /Users/evan/Programming/boost/library/include/boost/thread.hpp:24:0,
                 from main.cpp:1:
/Users/evan/Programming/boost/library/include/boost/thread/future.hpp: In static member function 'static void boost::detail::future_traits<T>::init(boost::detail::future_traits<T>::storage_type&, boost::detail::future_traits<T>::source_reference_type) [with T = MovableButNonCopyable, boost::detail::future_traits<T>::storage_type = boost::scoped_ptr<MovableButNonCopyable>, boost::detail::future_traits<T>::source_reference_type = const MovableButNonCopyable&]':
/Users/evan/Programming/boost/library/include/boost/thread/future.hpp:308:17:   instantiated from 'void boost::detail::future_object<T>::mark_finished_with_result_internal(boost::detail::future_object<T>::source_reference_type) [with T = MovableButNonCopyable, boost::detail::future_object<T>::source_reference_type = const MovableButNonCopyable&]'
/Users/evan/Programming/boost/library/include/boost/thread/future.hpp:325:17:   instantiated from 'void boost::detail::future_object<T>::mark_finished_with_result(boost::detail::future_object<T>::rvalue_source_type) [with T = MovableButNonCopyable, boost::detail::future_object<T>::rvalue_source_type = MovableButNonCopyable&&]'
/Users/evan/Programming/boost/library/include/boost/thread/future.hpp:1224:21:   instantiated from 'void boost::detail::task_object<R, F>::do_run() [with R = MovableButNonCopyable, F = main()::<lambda()>]'
main.cpp:12:1:   instantiated from here
/Users/evan/Programming/boost/library/include/boost/thread/future.hpp:239:17: error: use of deleted function 'MovableButNonCopyable::MovableButNonCopyable(const MovableButNonCopyable&)'
main.cpp:4:5: error: declared here

Unfortunately I do not have any other compilers that I can test it on right at the moment. When I get the chance, I will try a few more versions of GCC, Clang and MSVC. I will also see if I can find a way to reproduce the bug without using lambdas or defaulted/deleted functions. (I will report my findings some time this week).

[viboes]

The is an action point to use Boost.Move. Could we consider this as a duplicate of #6194 Adapt to Boost.Move.

[onlyone@…]

Replying to viboes:

The is an action point to use Boost.Move. Could we consider this as a duplicate of #6194 Adapt to Boost.Move.

#6174 does not involve the move emulation (as the bug occurs in C++11 mode). That said, I have spent the last week working on #6194, and any patch that fixes it will certainly also include a fix for this.

[viboes]

Replying to onlyone@…:

Replying to viboes:

The is an action point to use Boost.Move. Could we consider this as a duplicate of #6194 Adapt to Boost.Move.

#6174 does not involve the move emulation (as the bug occurs in C++11 mode). That said, I have spent the last week working on #6194, and any patch that fixes it will certainly also include a fix for this.

I have a patch that maybe could help waiting for #6194. Please could you try it?

[viboes]

Use of forward to try to solve the issue

[viboes]

Committed in trunk at revision [76543].

[viboes]

Committed in release branch at [78543]

[onlyone@…]

Patch for 6174, as well for the incorrect tests for 6174

[onlyone@…]

I'm reopening this as it has not been fixed in 1.50.0 or in trunk.

The original patchfile.patch is still an appropriate fix.

To ensure that this does not again fail to be fixed, I will attempt to explain very clearly what the issue is, what the fix is, and why I think it wasn't fixed earlier (so bear with me on the longwinded descriptions). I have also attached a patch that fixes both the bug and test case (test_6174.cpp) that previously failed to pick up this bug.

What the issue is:

The issue only occurs in C++11. The issue is that packaged_task incorrectly attempts to copy (instead of move) the return value of its function. This has two implications: packaged_task is performing an unnecessary copy of the return value, and worse, packaged_task cannot be used with return values that are not copyable (but movable).

What the fix is:

The problem happens when you attempt to call packaged_task::operator(). The call tree to the actual problem is as follows:

                      Executed code                                                     Called function
+-------------------------------------------------------------------+--------------------------------------------------------------
|(packaged_task<MovableButNonCopyale>(construct))()                 |packaged_task::operator()
|task->run()                                                        |detail::task_base::run()
|do_run()                                                           |task_object::do_run()  //Dispatched through a virtual call
|this->mark_finished_with_result(f())                               |future_object::mark_finished_with_result(rvalue_source_type result_)
|mark_finished_with_result_internal(result_) //Here is the bad line |//Should call `future_object::mark_finished_with_result_internal(rvalue_reference_type result_)`,
|                                                                   |//but doesn't because of the bad line. Instead calls:
|                                                                   |future_object::mark_finished_with_result_internal(source_reference_type result_)
|future_traits<T>::init(result,result_)                             |future_traits<T>::init(storage_type& storage,source_reference_type t)
|storage.reset(new T(t)); // Here is the incorrect copy             |
+-------------------------------------------------------------------+--------------------------------------------------------------

That is, the bad code is in future_object::mark_finished_with_result(rvalue_source_type result_). The broken version of future_object::mark_finished_with_result(rvalue_source_type result_) looks like this:

void mark_finished_with_result(rvalue_source_type result_)
{
    boost::lock_guard<boost::mutex> lock(mutex);
    mark_finished_with_result_internal(result_);
}

The call to mark_finished_with_result_internal(result_) picks the incorrect overload, because result_ is an lvalue. To fix this, it is necessary to add a cast (as is done in every other part of the code).

The fixed version looks like this:

void mark_finished_with_result(rvalue_source_type result_)
{
    boost::lock_guard<boost::mutex> lock(mutex);
    mark_finished_with_result_internal(static_cast<rvalue_source_type>(result_));
}

Problems with the original example:

The original example failed to consistently trigger the issue, because it did not include a call to packaged_task::operator() (so the broken code would sometimes not be instantiated). Here is a new example without that issue:

#include <boost/thread.hpp>
struct MovableButNonCopyable {
    MovableButNonCopyable() = default;
    MovableButNonCopyable(MovableButNonCopyable const&) = delete;
    MovableButNonCopyable& operator=(MovableButNonCopyable const&) = delete;
    MovableButNonCopyable(MovableButNonCopyable&&) = default;
    MovableButNonCopyable& operator=(MovableButNonCopyable&&) = default;
};
MovableButNonCopyable construct() { return MovableButNonCopyable(); }
int main()
{
    boost::packaged_task<MovableButNonCopyable> pt(construct);
    pt();
}

Problems with the test case:

The test case in trunk was totally broken. The main part of the test case looked like this:

int main() {
    boost::packaged_task<MovableButNonCopyable>(MovableButNonCopyable());
}

This is attempting to package up a MovableButNonCopyable as a task that returns a MovableButNonCopyable. MovableButNonCopyable is not callable, so this will never work. To make the test case work, it must be changed to:

MovableButNonCopyable construct() { return MovableButNonCopyable(); }
int main() {
    boost::packaged_task<MovableButNonCopyable> pt(construct);
    pt();
}

Problems with the test runner:
Even once the test case was fixed, it still did not correctly identify the issue, as there was an additional problem with the way in which the tests were being run.

The Jamfile for the Boost.Thread test suite causes the the -ansi flag to be added when building with clang or gcc. This overrides any -std=c++11 flags that may have been added, and means that the relevant part of test_6174.cpp is never run on these compilers (because the bug only occurs in C++11). To fix this, the Jamfile was modified so that it did not give the -ansi flag.

Other unrelated problems:

After the -ansi flag was removed from the test runner, I was able to run the Boost.Thread test suite with clang in C++11 configuration. Everything passed except for sync/mutual_exclusion/locks/unique_lock/obs/op_int_fail.cpp. It appears that Clang is incorrectly able to compile this test when in C++11 mode. While this is interesting, I did not investigate further, as it is unrelated to 6174.

Also, I did not understand the reasoning behind the -ansi flag being present. It was also present in the Jamfile for the main Boost.Thread build, but it did not seem necessary.

The solution:

I have attached a patch which modifies mark_finished_with_result, test_6174.cpp and the test Jamfile to fix the problems described above.

[viboes]

Thanks for the clear explanations. I missed completely the bug.

Committed in trunk revision 79980.

[viboes]

Committed revision 80040.