Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

When attaching a continuation to boost::future, the continuation is executed in a new thread(default), which will cause memory usage problem #156

Open
fengzhuye opened this issue Mar 16, 2017 · 13 comments

Comments

@fengzhuye
Copy link

fengzhuye commented Mar 16, 2017

In the example callee.cpp: one should not use it this

provide_future_add = session->provide("com.examples.calculator.add2", &add2).then(
                        [&](boost::future<autobahn::wamp_registration> registration) {
                       ...
                    });

but like this:

provide_future_add = session->provide("com.examples.calculator.add2", &add2).then(
                        boost::launch::deferred,
                        [&](boost::future<autobahn::wamp_registration> registration) {
                       ...
                    });
@fengzhuye
Copy link
Author

fengzhuye commented Mar 17, 2017

I guess i encountered this problem: https://svn.boost.org/trac/boost/ticket/12220

And that's a bug of Boost, which have been fixed.

Here is my code ( boost verison 1.59.0 with gcc 4.8.4 and ubuntu 14.04 ):

If you don't use boost::launch::deferred policy and you comment provide_future.get() then there will be a memory leak.

for(auto it=service_maps_.begin(); it!=service_maps_.end(); it++)
{
    auto provide_future = session_->provide(it->first, it->second).then(
//                            boost::launch::deferred, 
            [&](boost::future<autobahn::wamp_registration> registration) {
            try {
                registration.get().id() ;
            }
            catch (const std::exception& e) {
                RTT::Logger::In in("wamp_backend");
                RTT::log(RTT::Error) << "register callee: " << it->first << " failed!" << RTT::nlog();
                io_.stop();
                return;
            }
    });
//                    provide_future.get();
}

@oberstet
Copy link
Contributor

oberstet commented Mar 17, 2017

First, thanks for giving more context and explaining. I reopened the issue.

So this is fixed in Boost 1.62.0 and the memory leak will only hit when using an older version?

Because, if this is the case, then we can simply require 1.62.0 or higher rather than changing all our examples with this workaround / hack .. which makes the code look rather ugly.

@oberstet
Copy link
Contributor

@oberstet
Copy link
Contributor

oberstet commented Mar 17, 2017

Ahhh .. one of the answers in above SO post reveals more:

"It is customizable whether boost::future continuation should be executed in a new thread or in the calling thread."

boost::launch::deferred: "That should run the continuation in the same thread."


IMO it is highly disturbing and unexpected that this is NOT the default!

I don't want random continuations spawn new threads all the time. I don't want threads at all (unless I explicitly need them because I am doing CPU bound stuff).

This sucks big time.

In fact, this might be related to #146

Not only does Boost spawn threads for each and every continuation, but it then fails to cleanup the mess properly. Oh well, C++ has still a long way to go in the async world.

I recently learned that co-routines didn't make it into C++ 2017, and will not come before C++ 2020. This is a joke. Come on .. we have usable co-routines in Python/Twisted for 10+ years now;)

@oberstet
Copy link
Contributor

@fengzhuye so can you confirm that boost::launch::deferred is needed even with the latest Boost version to make the continuation run on the current thread? If so, I am willing to change all examples.

@fengzhuye
Copy link
Author

fengzhuye commented Mar 17, 2017

This is my blog :), amazing you found it: http://blog.csdn.net/gw569453350game/article/details/62426378

I am not sure whether the boost::future have fixed the memory leak bug(cause i didn't test it). But make the boost::launch::deferred to default policy will be a good option (at least for the wamp_session::provide function ). Or we can warn the users about the possible risks when use the default boost::launch::async policy, because it did happen to me.

@oberstet
Copy link
Contributor

Your blog is easy to find, as you linked it on your GitHub user page. Problem is: I cannot read it .. other than code .. ha;) What are you working on / using AutobahnC++ for btw?

@fengzhuye
Copy link
Author

We use WAMP as a tool to connect the ui and embeded robot controller. Besides, we use the bonefish router, which is very good and much faster.

Cheers,

@oberstet
Copy link
Contributor

Ah, interesting.

FWIW, I doubt Bonefish is faster than Crossbar.io (on PyPy) ..

@oberstet
Copy link
Contributor

oberstet commented Mar 19, 2017

@fengzhuye put differently, your proposal of adding boost::launch::deferred everywhere would work, but it is introducing a lot of syntax noise. I would like to have a global switch that runs continuations on the same thread by default and without changing dozens and hundreds of code locations.

Do you know how to do that?

http://stackoverflow.com/questions/22934575/why-does-boostfuturetthen-spawn-a-new-thread#comment72882245_42157631

@fengzhuye
Copy link
Author

@oberstet It seems there is no such global option:

When the launch policy is launch::none the continuation is called on an unspecified thread of execution.
When the executor or launch policy is not provided (first overload) is if as if launch::none was specified.

http://www.boost.org/doc/libs/1_63_0/doc/html/thread/synchronization.html#thread.synchronization.futures.reference.unique_future.then

@oberstet
Copy link
Contributor

@fengzhuye I was afraid there isn't a global option .. thanks for confirming!

Also: sorry, I didn't get the whole picture when I first responded to you.

It seems, we don't have a lot of options, and all of them are not pretty:

  1. add boost::launch::deferred policy all over the place (what you suggested originally)
  2. migrate away from Boost/ASIO to standalone ASIO - assuming the latter would allow configuring this globally (which I also don't know for sure).
  3. wait for C++20, which fixes this?
  4. write a wrapper around boost::future that decorates automatically as suggested here

@ecorm
Copy link

ecorm commented Jan 27, 2018

Asio was updated in Boost 1.66 release. There's mention of executors in this new documentation page: http://www.boost.org/doc/libs/1_66_0/doc/html/boost_asio/net_ts.html

I'm too busy with work/life right now to investigate any deeper. Coroutines and the traditional callback interface have been working well for us so far at work; we don't use use futures.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants