std::packaged_task trunk problem

std::packaged_task trunk problem

[Lars Gullik Bjønnes]

I am trying out trunk and the std::packaged_task in C++11. I found an
example on usage at
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2008/n2709.html
but are unable to make it work.

I wonder if it is the example or if it is the libstdc++ playing up.

#include <iostream>
#include <future>
#include <thread>

template<typename F>
std::future<typename std::result_of<F()>::type> spawn_task(F f)
{
    typedef typename std::result_of<F()>::type result_type;
    std::packaged_task<result_type()> task(std::move(f));
    std::future<result_type> res(task.get_future());
    std::thread(std::move(task));
    return res;
}


int get_res()
{
	return 42;
}


int main()
{
	auto f = spawn_task(get_res);
	std::cout << "Res = " << f.get() << std::endl;
}


$ /opt/gcc/gcc-trunk/bin/g++ -Wall -Wextra -O2 -std=gnu++0x packaged_task_example.cpp 
In file included from /opt/gcc/gcc-trunk/lib/gcc/x86_64-unknown-linux-gnu/4.7.0/../../../../include/c++/4.7.0/functional:56:0,
                 from /opt/gcc/gcc-trunk/lib/gcc/x86_64-unknown-linux-gnu/4.7.0/../../../../include/c++/4.7.0/future:38,
                 from packaged_task_example.cpp:2:
/opt/gcc/gcc-trunk/lib/gcc/x86_64-unknown-linux-gnu/4.7.0/../../../../include/c++/4.7.0/tuple: In instantiation of ‘struct std::_Tuple_impl<0ul, std::packaged_task<int()> >’:
/opt/gcc/gcc-trunk/lib/gcc/x86_64-unknown-linux-gnu/4.7.0/../../../../include/c++/4.7.0/tuple:398:11:   required from ‘class std::tuple<std::packaged_task<int()> >’
/opt/gcc/gcc-trunk/lib/gcc/x86_64-unknown-linux-gnu/4.7.0/../../../../include/c++/4.7.0/functional:1546:39:   required from ‘struct std::_Bind_simple<std::packaged_task<int()>()>’
/opt/gcc/gcc-trunk/lib/gcc/x86_64-unknown-linux-gnu/4.7.0/../../../../include/c++/4.7.0/thread:133:9:   required from ‘std::thread::thread(_Callable&&, _Args&& ...) [with _Callable = std::packaged_task<int()>; _Args = {}]’
packaged_task_example.cpp:12:5:   required from ‘std::future<typename std::result_of<F()>::type> spawn_task(F) [with F = int (*)(); typename std::result_of<F()>::type = int]’
packaged_task_example.cpp:25:29:   required from here
/opt/gcc/gcc-trunk/lib/gcc/x86_64-unknown-linux-gnu/4.7.0/../../../../include/c++/4.7.0/tuple:274:17: error: ‘constexpr std::_Tuple_impl<_Idx, _Head, _Tail ...>::_Tuple_impl(const std::_Tuple_impl<_Idx, _Head, _Tail ...>&) [with long unsigned int _Idx = 0ul; _Head = std::packaged_task<int()>; _Tail = {}; std::_Tuple_impl<_Idx, _Head, _Tail ...> = std::_Tuple_impl<0ul, std::packaged_task<int()> >]’ declared to take const reference, but implicit declaration would take non-const

Thanks,

-- 
   Lgb

Re: std::packaged_task trunk problem

[Jonathan Wakely]

On 1 November 2011 10:55, Lars Gullik Bjønnes wrote:
>
> I am trying out trunk and the std::packaged_task in C++11. I found an
> example on usage at
> http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2008/n2709.html
> but are unable to make it work.
>
> I wonder if it is the example or if it is the libstdc++ playing up.

Possibly, but that paper's quite old and the example might be invalid
according to the final standard.  I'll look into it later today.

Re: std::packaged_task trunk problem

[Jonathan Wakely]

On 1 November 2011 15:03, Jonathan Wakely wrote:
> On 1 November 2011 10:55, Lars Gullik Bjønnes wrote:
>>
>> I am trying out trunk and the std::packaged_task in C++11. I found an
>> example on usage at
>> http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2008/n2709.html
>> but are unable to make it work.
>>
>> I wonder if it is the example or if it is the libstdc++ playing up.
>
> Possibly, but that paper's quite old and the example might be invalid
> according to the final standard.  I'll look into it later today.
>

Aha, this is http://lwg.github.com/issues/lwg-active.html#2067

Although it's only in READY status I'll apply the fix to libstdc++ tonight.

Re: std::packaged_task trunk problem

[Lars Gullik Bjønnes]

On Tue, Nov 1, 2011 at 16:08, Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
> On 1 November 2011 15:03, Jonathan Wakely wrote:
>> On 1 November 2011 10:55, Lars Gullik Bjønnes wrote:
>>>
>>> I am trying out trunk and the std::packaged_task in C++11. I found an
>>> example on usage at
>>> http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2008/n2709.html
>>> but are unable to make it work.
>>>
>>> I wonder if it is the example or if it is the libstdc++ playing up.
>>
>> Possibly, but that paper's quite old and the example might be invalid
>> according to the final standard.  I'll look into it later today.
>>
>
> Aha, this is http://lwg.github.com/issues/lwg-active.html#2067
>
> Although it's only in READY status I'll apply the fix to libstdc++ tonight.

Thanks a lot for having a look at this.

-- 
        Lgb

Re: std::packaged_task trunk problem

[Jonathan Wakely]

On 1 November 2011 15:12, Lars Gullik Bjønnes wrote:
>>
>> Aha, this is http://lwg.github.com/issues/lwg-active.html#2067
>>
>> Although it's only in READY status I'll apply the fix to libstdc++ tonight.
>
> Thanks a lot for having a look at this.

I've fixed it in the svn trunk by implementing LWG 2067, and working
around some new issues I found in <future>.

N.B. your example is still broken, because the std::thread is not
explicitly joined or detached so will call std::terminate in its
destructor.  That's due to a change to std::thread that was made after
the packaged_task paper you got the example from.  It can be fixed by
detaching the thread (which is safe in this specific case because the
thread doesn't refer to anything that will go out of scope before it
finishes):

  std::thread(std::move(task)).detach();

Re: std::packaged_task trunk problem

[Lars Gullik Bjønnes]

On Wed, Nov 2, 2011 at 01:56, Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
> On 1 November 2011 15:12, Lars Gullik Bjønnes wrote:
>>>
>>> Aha, this is http://lwg.github.com/issues/lwg-active.html#2067
>>>
>>> Although it's only in READY status I'll apply the fix to libstdc++ tonight.
>>
>> Thanks a lot for having a look at this.
>
> I've fixed it in the svn trunk by implementing LWG 2067, and working
> around some new issues I found in <future>.
>
> N.B. your example is still broken, because the std::thread is not
> explicitly joined or detached so will call std::terminate in its
> destructor.  That's due to a change to std::thread that was made after
> the packaged_task paper you got the example from.  It can be fixed by
> detaching the thread (which is safe in this specific case because the
> thread doesn't refer to anything that will go out of scope before it
> finishes):
>
>  std::thread(std::move(task)).detach();

Ok. Thanks, that work fine.

Now of to next problem on how to store a std::packaged_task in a container.



-- 
        Lgb

Re: std::packaged_task trunk problem

[Jonathan Wakely]

On 2 November 2011 02:01, Lars Gullik Bjønnes wrote:
>
> Now of to next problem on how to store a std::packaged_task in a container.

That should work because packaged_task is nothrow move constructible,
but it looks as though there may be a problem with the
_GLIBCXX_MAKE_MOVE_IF_NOEXCEPT_ITERATOR code ... I'd better look into
that too.

Re: std::packaged_task trunk problem

[Jonathan Wakely]

On 2 November 2011 09:06, Jonathan Wakely wrote:
> On 2 November 2011 02:01, Lars Gullik Bjønnes wrote:
>>
>> Now of to next problem on how to store a std::packaged_task in a container.
>
> That should work because packaged_task is nothrow move constructible,
> but it looks as though there may be a problem with the
> _GLIBCXX_MAKE_MOVE_IF_NOEXCEPT_ITERATOR code ... I'd better look into
> that too.
>

is_nothrow_move_constructible<packaged_task<T>>::value is false,
because packaged_task's destructor is not noexcept.  I'm not sure yet
if that's correct or not...

Re: std::packaged_task trunk problem

[Jonathan Wakely]

On 2 November 2011 10:03, Jonathan Wakely wrote:
> On 2 November 2011 09:06, Jonathan Wakely wrote:
>> On 2 November 2011 02:01, Lars Gullik Bjønnes wrote:
>>>
>>> Now of to next problem on how to store a std::packaged_task in a container.
>>
>> That should work because packaged_task is nothrow move constructible,
>> but it looks as though there may be a problem with the
>> _GLIBCXX_MAKE_MOVE_IF_NOEXCEPT_ITERATOR code ... I'd better look into
>> that too.
>>
>
> is_nothrow_move_constructible<packaged_task<T>>::value is false,
> because packaged_task's destructor is not noexcept.  I'm not sure yet
> if that's correct or not...

It's correct that
is_nothrow_move_constructible<packaged_task<T>>::value is false, but
that shouldn't prevent it being stored in a container. I'll file a
bugzilla report.

Re: std::packaged_task trunk problem

[Jonathan Wakely]

On 2 November 2011 11:22, Jonathan Wakely wrote:
> On 2 November 2011 10:03, Jonathan Wakely wrote:
>> On 2 November 2011 09:06, Jonathan Wakely wrote:
>>> On 2 November 2011 02:01, Lars Gullik Bjønnes wrote:
>>>>
>>>> Now of to next problem on how to store a std::packaged_task in a container.
>>>
>>> That should work because packaged_task is nothrow move constructible,
>>> but it looks as though there may be a problem with the
>>> _GLIBCXX_MAKE_MOVE_IF_NOEXCEPT_ITERATOR code ... I'd better look into
>>> that too.
>>>
>>
>> is_nothrow_move_constructible<packaged_task<T>>::value is false,
>> because packaged_task's destructor is not noexcept.  I'm not sure yet
>> if that's correct or not...
>
> It's correct that
> is_nothrow_move_constructible<packaged_task<T>>::value is false, but
> that shouldn't prevent it being stored in a container. I'll file a
> bugzilla report.

Bah, I think my first test was flawed - it works storing them in
containers if you only use the members that support MoveInsertable

i.e. this fails as it requires CopyInsertable:
vector<packaged_task<void()>> v{ packaged_task<void()>{} };

this is OK, it only requires MoveInsertable:
vector<packaged_task<void()>> v;
v.push_back( packaged_task<void()>{} );

Re: std::packaged_task trunk problem

[Lars Gullik Bjønnes]

On Wed, Nov 2, 2011 at 13:01, Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
> On 2 November 2011 11:22, Jonathan Wakely wrote:
>> On 2 November 2011 10:03, Jonathan Wakely wrote:
>>> On 2 November 2011 09:06, Jonathan Wakely wrote:
>>>> On 2 November 2011 02:01, Lars Gullik Bjønnes wrote:
>>>>>
>>>>> Now of to next problem on how to store a std::packaged_task in a container.
>>>>
>>>> That should work because packaged_task is nothrow move constructible,
>>>> but it looks as though there may be a problem with the
>>>> _GLIBCXX_MAKE_MOVE_IF_NOEXCEPT_ITERATOR code ... I'd better look into
>>>> that too.
>>>>
>>>
>>> is_nothrow_move_constructible<packaged_task<T>>::value is false,
>>> because packaged_task's destructor is not noexcept.  I'm not sure yet
>>> if that's correct or not...
>>
>> It's correct that
>> is_nothrow_move_constructible<packaged_task<T>>::value is false, but
>> that shouldn't prevent it being stored in a container. I'll file a
>> bugzilla report.
>
> Bah, I think my first test was flawed - it works storing them in
> containers if you only use the members that support MoveInsertable
>
> i.e. this fails as it requires CopyInsertable:
> vector<packaged_task<void()>> v{ packaged_task<void()>{} };
>
> this is OK, it only requires MoveInsertable:
> vector<packaged_task<void()>> v;
> v.push_back( packaged_task<void()>{} );

Great, that works.

I tried to put the packaged_task into a
std::vector<std::function<void()>>, that fails.
I had to create a wrapper around the packaged_task to make it work.

-- 
        Lgb

Re: std::packaged_task trunk problem

[Jonathan Wakely]

On 3 November 2011 13:13, Lars Gullik Bjønnes wrote:
>
> I tried to put the packaged_task into a
> std::vector<std::function<void()>>, that fails.
> I had to create a wrapper around the packaged_task to make it work.

Yes, std::function can only hold CopyConstructible objects:
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2011/n3313.html#1287

A copy constructor is required even if it will actually never be
needed. In that defect report I said it would be a terrible idea for
std::function to throw if you tried to copy it when it contained a
non-copyable target object, which I think is true for std::function in
general, but that doesn't stop you providing that behaviour via a
wrapper if you want it.

Something like ...

template<typename Moveable>
struct PretendToBeCopyable
{
  explicit PretendToBeCopyable(Moveable&& m) : m(m) {}
  PretendToBeCopyable(PretendToBeCopyable&&) = default;
  PretendToBeCopyable(const PretendToBeCopyable&)
  { throw std::logic_error("No way, Jose"); }
  // ...
  private:
    Moveable m;
};

Re: std::packaged_task trunk problem

[Jonathan Wakely]

On 3 November 2011 16:17, Jonathan Wakely wrote:
>
> Something like ...
>
> template<typename Moveable>
> struct PretendToBeCopyable
> {
>  explicit PretendToBeCopyable(Moveable&& m) : m(m) {}

Oops, that mem-initializer would need to be m(std::move(m))

Re: std::packaged_task trunk problem

[Lars Gullik Bjønnes]

Jonathan Wakely <jwakely.gcc@gmail.com> writes:

| On 3 November 2011 16:17, Jonathan Wakely wrote:
>>
>> Something like ...
>>
>> template<typename Moveable>
>> struct PretendToBeCopyable
>> {
>>  explicit PretendToBeCopyable(Moveable&& m) : m(m) {}
>
| Oops, that mem-initializer would need to be m(std::move(m))

This is the wrapper I ended up with. I had to remove the throw, it
triggered all the time.

template<typename Moveable>
struct PretendToBeCopyable
{
        explicit PretendToBeCopyable(Moveable&& m) : m(std::move(m)) {}
        PretendToBeCopyable(PretendToBeCopyable&&) = default;
        PretendToBeCopyable(PretendToBeCopyable& p)
                : m(std::move(p.m)) {}
        void operator()() { m(); }
private:
        Moveable m;
};

}

-- 
   Lgb

Re: std::packaged_task trunk problem

[Jonathan Wakely]

On 7 November 2011 09:26, Lars Gullik Bjønnes wrote:
>
> This is the wrapper I ended up with. I had to remove the throw, it
> triggered all the time.

What if you give PretendToBeCopyable a noexcept destructor?
That should ensure the container will move it rather than copy it,
whenever possible.

> template<typename Moveable>
> struct PretendToBeCopyable
> {
>        explicit PretendToBeCopyable(Moveable&& m) : m(std::move(m)) {}
>        PretendToBeCopyable(PretendToBeCopyable&&) = default;
>        PretendToBeCopyable(PretendToBeCopyable& p)
>                : m(std::move(p.m)) {}
>        void operator()() { m(); }

          ~PretendToBeCopyable() noexcept = default;

> private:
>        Moveable m;
> };
>
> }

Re: std::packaged_task trunk problem

[Jonathan Wakely]

On 7 November 2011 09:51, Jonathan Wakely wrote:
> On 7 November 2011 09:26, Lars Gullik Bjønnes wrote:
>>
>> This is the wrapper I ended up with. I had to remove the throw, it
>> triggered all the time.
>
> What if you give PretendToBeCopyable a noexcept destructor?
> That should ensure the container will move it rather than copy it,
> whenever possible.

And of course also make the copy constructor throw again, or you won't
know if the nothrow destructor made any difference or not.

Re: std::packaged_task trunk problem

[Lars Gullik Bjønnes]

Jonathan Wakely <jwakely.gcc@gmail.com> writes:

| On 7 November 2011 09:51, Jonathan Wakely wrote:
>> On 7 November 2011 09:26, Lars Gullik Bjønnes wrote:
>>>
>>> This is the wrapper I ended up with. I had to remove the throw, it
>>> triggered all the time.
>>
>> What if you give PretendToBeCopyable a noexcept destructor?
>> That should ensure the container will move it rather than copy it,
>> whenever possible.
>
| And of course also make the copy constructor throw again, or you won't
| know if the nothrow destructor made any difference or not.

Making the destructor noexcept does not even compile.
Below is the example code that I am toying with now, not that the copy
constructor seems to never be called, probably sine vec never is
resized. In my larger code, the throw triggers always.


#include <iostream>
#include <future>
#include <thread>
#include <vector>

std::vector<std::function<void()>> vec;

template<typename Moveable>
struct PretendToBeCopyable
{
	explicit PretendToBeCopyable(Moveable&& m) : m(std::move(m)) {}
	//~PretendToBeCopyable() noexcept = default;
	PretendToBeCopyable(PretendToBeCopyable&&) = default;
	PretendToBeCopyable(PretendToBeCopyable& p)
		: m(std::move(p.m))
	{
		// This would never throw in this example sin vec is never
		// resized.
		// throw 5;
	}
	void operator()() { m(); }
private:
	Moveable m;
};

template<typename F>
std::future<typename std::result_of<F()>::type> spawn_task(F f)
{
    typedef typename std::result_of<F()>::type result_type;
    typedef std::packaged_task<result_type()> task_type;

    task_type task(std::move(f));
    std::future<result_type> res = task.get_future();

    vec.push_back(PretendToBeCopyable<task_type>(std::move(task)));
    
    std::thread([]()
		{
			auto task = std::move(vec.back());
			vec.pop_back();
			task();
		}
	    ).detach();
    
    return std::move(res);
}


double get_res()
{
	return 42.2;
}


int main()
{
	auto f = spawn_task(get_res);
	std::cout << "Res = " << f.get() << std::endl;
}

Re: std::packaged_task trunk problem

[Jonathan Wakely]

On 7 November 2011 10:08, Lars Gullik Bjønnes wrote:
>
> Making the destructor noexcept does not even compile.

Oops, of course, sorry - it can't be defaulted because the default
definition would not be noexcept.  This works:

        ~PretendToBeCopyable() noexcept { }

Re: std::packaged_task trunk problem

[Lars Gullik Bjønnes]

Jonathan Wakely <jwakely.gcc@gmail.com> writes:

| On 7 November 2011 10:08, Lars Gullik Bjønnes wrote:
>>
>> Making the destructor noexcept does not even compile.
>
| Oops, of course, sorry - it can't be defaulted because the default
| definition would not be noexcept.  This works:
>
|         ~PretendToBeCopyable() noexcept { }

Yes it compiles. The copy constructor is still run, but if having that
throw I still get into std::terminate.

-- 
   Lgb

Re: std::packaged_task trunk problem

[Jonathan Wakely]

On 7 November 2011 10:46, Lars Gullik Bjønnes wrote:
>
> Yes it compiles. The copy constructor is still run, but if having that
> throw I still get into std::terminate.

The reason the copy constructor does still get called is because
std::function doesn't have a noexcept move constructor, so resizing
the vector will copy the function (which copies its target
PretendToBeCopyable) instead of moving it.

I hadn't realised how hard it is to make this work with the final
C++11 rules :-\

Re: std::packaged_task trunk problem

[Lars Gullik Bjønnes]

Jonathan Wakely <jwakely.gcc@gmail.com> writes:

| On 7 November 2011 10:46, Lars Gullik Bjønnes wrote:
>>
>> Yes it compiles. The copy constructor is still run, but if having that
>> throw I still get into std::terminate.
>
| The reason the copy constructor does still get called is because
| std::function doesn't have a noexcept move constructor, so resizing
| the vector will copy the function (which copies its target
| PretendToBeCopyable) instead of moving it.
>
| I hadn't realised how hard it is to make this work with the final
| C++11 rules :-\

I guess I am into more fun now that I want my spawn_task to take
arguments for the spawned function as well. :-)

-- 
   Lgb