crossbario / autobahn-cpp

WAMP for C++ in Boost/Asio
https://crossbar.io/autobahn
Boost Software License 1.0
251 stars 104 forks source link

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 7 years ago

fengzhuye commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

Here is more context:

oberstet commented 7 years ago

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 https://github.com/crossbario/autobahn-cpp/issues/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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

Ah, interesting.

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

oberstet commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

@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 commented 6 years ago

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.