Boost C++ Libraries: Ticket #6174: packaged_task doesn't correctly handle moving results

attachment set

onlyone@… — Fri, 25 Nov 2011 12:38:44 GMT

attachment → patchfile.patch

Fix for #6174

summary changed

onlyone@… — Fri, 25 Nov 2011 12:57:54 GMT

summary packaged_task does correctly handle moving results → [thread] packaged_task does correctly handle moving results

owner, status changed

viboes — Fri, 02 Dec 2011 20:47:05 GMT

owner changed from Anthony Williams to viboes
status new → assigned

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());

cc changed

viboes — Fri, 02 Dec 2011 20:47:33 GMT

cc viboes added

onlyone@… — Sat, 03 Dec 2011 15:23:43 GMT

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 — Sun, 11 Dec 2011 11:40:48 GMT

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

onlyone@… — Sun, 11 Dec 2011 12:07:03 GMT

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.

summary changed

viboes — Sun, 11 Dec 2011 14:12:52 GMT

summary [thread] packaged_task does correctly handle moving results → packaged_task doesn't correctly handle moving results

viboes — Mon, 19 Dec 2011 18:37:01 GMT

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?

attachment set

viboes — Mon, 19 Dec 2011 18:38:57 GMT

attachment → detail_thread.hpp.diff

Use of forward to try to solve the issue

type changed

viboes — Thu, 29 Dec 2011 14:16:32 GMT

type Bugs → Patches

milestone changed

viboes — Sat, 07 Jan 2012 23:06:18 GMT

milestone To Be Determined → Boost 1.49.0

viboes — Tue, 17 Jan 2012 06:40:06 GMT

Committed in trunk at revision [76543].

status, milestone changed; resolution set

viboes — Mon, 28 May 2012 15:44:10 GMT

status assigned → closed
resolution → fixed
milestone Boost 1.49.0 → Boost 1.50.0

Committed in release branch at [78543]

attachment set

onlyone@… — Thu, 19 Jul 2012 13:21:52 GMT

attachment → newpatch.patch

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

onlyone@… — Thu, 19 Jul 2012 13:25:32 GMT

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.

status, milestone changed; resolution deleted

onlyone@… — Thu, 19 Jul 2012 13:26:28 GMT

status closed → reopened
resolution fixed
milestone Boost 1.50.0 → Boost 1.51.0

status changed

viboes — Sun, 12 Aug 2012 16:38:32 GMT

status reopened → new

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

Committed in trunk revision 79980.

milestone changed

viboes — Sun, 12 Aug 2012 16:38:52 GMT

milestone Boost 1.51.0 → Boost 1.52.0

status changed

viboes — Sun, 12 Aug 2012 19:02:56 GMT

status new → assigned

status, milestone changed; resolution set

viboes — Wed, 15 Aug 2012 09:56:40 GMT

status assigned → closed
resolution → fixed
milestone Boost 1.52.0 → Boost 1.51.0

Committed revision 80040.