Re: [Boost-bugs] [Boost C++ Libraries] #6174: packaged_task doesn't correctly handle moving results

Subject: Re: [Boost-bugs] [Boost C++ Libraries] #6174: packaged_task doesn't correctly handle moving results
From: Boost C++ Libraries (noreply_at_[hidden])
Date: 2012-07-19 13:25:32

#6174: packaged_task doesn't correctly handle moving results
--------------------------------------+-------------------------------------
  Reporter:  onlyone@…                |       Owner:  viboes            
      Type:  Patches                  |      Status:  closed            
 Milestone:  Boost 1.50.0             |   Component:  thread            
   Version:  Boost Development Trunk  |    Severity:  Showstopper       
Resolution:  fixed                    |    Keywords:  move packaged_task
--------------------------------------+-------------------------------------

Comment (by 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:'''[[BR]]

 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:'''[[BR]]

 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:'''[[BR]]

 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:'''[[BR]]

 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:'''[[BR]]
 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.[[BR]]

 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:'''[[BR]]

 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:'''[[BR]]

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

-- 
Ticket URL: <https://svn.boost.org/trac/boost/ticket/6174#comment:13>
Boost C++ Libraries <http://www.boost.org/>
Boost provides free peer-reviewed portable C++ source libraries.