Modify

Ticket #6174 (closed Patches: fixed)

Opened 2 years ago

Last modified 21 months ago

packaged_task doesn't correctly handle moving results

Reported by: onlyone@… Owned by: viboes
Milestone: Boost 1.51.0 Component: thread
Version: Boost Development Trunk Severity: Showstopper
Keywords: move packaged_task Cc: anthonyw, viboes

Description

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.

Attachments

patchfile.patch Download (564 bytes) - added by onlyone@… 2 years ago.
Fix for #6174
detail_thread.hpp.diff Download (3.3 KB) - added by viboes 2 years ago.
Use of forward to try to solve the issue
newpatch.patch Download (3.6 KB) - added by onlyone@… 21 months ago.
Patch for 6174, as well for the incorrect tests for 6174

Change History

Changed 2 years ago by onlyone@…

Fix for #6174

comment:1 Changed 2 years ago by onlyone@…

  • Summary changed from packaged_task does correctly handle moving results to [thread] packaged_task does correctly handle moving results

comment:2 follow-up: ↓ 4 Changed 2 years ago by viboes

  • Owner changed from anthonyw to viboes
  • Status changed from new to 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());

comment:3 Changed 2 years ago by viboes

  • Cc viboes added

comment:4 in reply to: ↑ 2 Changed 2 years ago by 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).

comment:5 follow-up: ↓ 6 Changed 2 years ago by viboes

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

comment:6 in reply to: ↑ 5 ; follow-up: ↓ 8 Changed 2 years ago by 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.

comment:7 Changed 2 years ago by viboes

  • Summary changed from [thread] packaged_task does correctly handle moving results to packaged_task doesn't correctly handle moving results

comment:8 in reply to: ↑ 6 Changed 2 years ago by 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?

Changed 2 years ago by viboes

Use of forward to try to solve the issue

comment:9 Changed 2 years ago by viboes

  • Type changed from Bugs to Patches

comment:10 Changed 2 years ago by viboes

  • Milestone changed from To Be Determined to Boost 1.49.0

comment:11 Changed 2 years ago by viboes

Committed in trunk at revision [76543].

comment:12 Changed 2 years ago by viboes

  • Status changed from assigned to closed
  • Resolution set to fixed
  • Milestone changed from Boost 1.49.0 to Boost 1.50.0

Committed in release branch at [78543]

Changed 21 months ago by onlyone@…

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

comment:13 Changed 21 months ago 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:

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.

comment:14 Changed 21 months ago by onlyone@…

  • Status changed from closed to reopened
  • Resolution fixed deleted
  • Milestone changed from Boost 1.50.0 to Boost 1.51.0

comment:15 Changed 21 months ago by viboes

  • Status changed from reopened to new

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

Committed in trunk revision 79980.

comment:16 Changed 21 months ago by viboes

  • Milestone changed from Boost 1.51.0 to Boost 1.52.0

comment:17 Changed 21 months ago by viboes

  • Status changed from new to assigned

comment:18 Changed 21 months ago by viboes

  • Status changed from assigned to closed
  • Resolution set to fixed
  • Milestone changed from Boost 1.52.0 to Boost 1.51.0

Committed revision 80040.

View

Add a comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
The resolution will be deleted. Next status will be 'reopened'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.