From 7e42d4b3e8f478b65956776f2654615399276e16 Mon Sep 17 00:00:00 2001 From: Daniel Larimer Date: Tue, 18 Aug 2015 11:36:50 -0400 Subject: [PATCH] Fix #242 - witness node crash Rather than using futures and waiting in the destructor, the APIs now use enable_shared_from_this and the lambda captures a shared pointer to the API to prevent it from going out of scope. As a result the destructor can not be called while there is a pending async operation which removes the need to wait in the destructor and thereby removing the potential for an exception to be thrown causing this crash. --- libraries/app/api.cpp | 40 ++++++++-------------- libraries/app/include/graphene/app/api.hpp | 6 ++-- 2 files changed, 17 insertions(+), 29 deletions(-) diff --git a/libraries/app/api.cpp b/libraries/app/api.cpp index 8c8f4512..c0625711 100644 --- a/libraries/app/api.cpp +++ b/libraries/app/api.cpp @@ -50,24 +50,6 @@ namespace graphene { namespace app { database_api::~database_api() { elog("freeing database api ${x}", ("x",int64_t(this)) ); - try { - if(_broadcast_changes_complete.valid()) - { - _broadcast_changes_complete.cancel(); - ilog( "waiting.."); - _broadcast_changes_complete.wait(); - } - if(_broadcast_removed_complete.valid()) - { - _broadcast_removed_complete.cancel(); - ilog( "waiting.."); - _broadcast_removed_complete.wait(); - } - ilog( "done" ); - } catch (const fc::exception& e) - { - wlog("${e}", ("e",e.to_detail_string())); - } } fc::variants database_api::get_objects(const vector& ids)const @@ -610,15 +592,18 @@ namespace graphene { namespace app { { if( _callbacks.size() ) { + /// we need to ensure the database_api is not deleted for the life of the async operation + auto capture_this = shared_from_this(); for( uint32_t trx_num = 0; trx_num < b.transactions.size(); ++trx_num ) { const auto& trx = b.transactions[trx_num]; auto id = trx.id(); auto itr = _callbacks.find(id); - auto block_num = b.block_num(); if( itr != _callbacks.end() ) { - fc::async( [=](){ itr->second( fc::variant(transaction_confirmation{ id, block_num, trx_num, trx}) ); } ); + auto block_num = b.block_num(); + auto& callback = _callbacks.find(id)->second; + fc::async( [capture_this,this,id,block_num,trx_num,trx,callback](){ callback( fc::variant(transaction_confirmation{ id, block_num, trx_num, trx}) ); } ); } } } @@ -807,6 +792,9 @@ namespace graphene { namespace app { void database_api::on_objects_removed( const vector& objs ) { + /// we need to ensure the database_api is not deleted for the life of the async operation + auto capture_this = shared_from_this(); + if( _account_subscriptions.size() ) { map > broadcast_queue; @@ -823,7 +811,7 @@ namespace graphene { namespace app { if( broadcast_queue.size() ) { - _broadcast_removed_complete = fc::async([=](){ + fc::async([capture_this,broadcast_queue,this](){ for( const auto& item : broadcast_queue ) { auto sub = _account_subscriptions.find(item.first); @@ -848,7 +836,7 @@ namespace graphene { namespace app { } if( broadcast_queue.size() ) { - _broadcast_removed_complete = fc::async([=](){ + fc::async([capture_this,this,broadcast_queue](){ for( const auto& item : broadcast_queue ) { auto sub = _market_subscriptions.find(item.first); @@ -904,12 +892,12 @@ namespace graphene { namespace app { } } + auto capture_this = shared_from_this(); - /// TODO: consider making _broadcast_changes_complete a deque and /// pushing the future back / popping the prior future if it is complete. /// if a connection hangs then this could get backed up and result in /// a failure to exit cleanly. - _broadcast_changes_complete = fc::async([=](){ + fc::async([capture_this,this,broadcast_queue,market_broadcast_queue,my_objects](){ for( const auto& item : broadcast_queue ) { edump( (item) ); @@ -980,7 +968,9 @@ namespace graphene { namespace app { if(_market_subscriptions.count(market)) subscribed_markets_ops[market].push_back(std::make_pair(op.op, op.result)); } - fc::async([=](){ + /// we need to ensure the database_api is not deleted for the life of the async operation + auto capture_this = shared_from_this(); + fc::async([this,capture_this,subscribed_markets_ops](){ for(auto item : subscribed_markets_ops) { auto itr = _market_subscriptions.find(item.first); diff --git a/libraries/app/include/graphene/app/api.hpp b/libraries/app/include/graphene/app/api.hpp index bf848a12..04ad5cfb 100644 --- a/libraries/app/include/graphene/app/api.hpp +++ b/libraries/app/include/graphene/app/api.hpp @@ -54,7 +54,7 @@ namespace graphene { namespace app { * read-only; all modifications to the database must be performed via transactions. Transactions are broadcast via * the @ref network_broadcast_api. */ - class database_api + class database_api : public std::enable_shared_from_this { public: database_api(graphene::chain::database& db); @@ -383,8 +383,6 @@ namespace graphene { namespace app { void on_objects_removed(const vector& objs); void on_applied_block(); - fc::future _broadcast_changes_complete; - fc::future _broadcast_removed_complete; boost::signals2::scoped_connection _change_connection; boost::signals2::scoped_connection _removed_connection; boost::signals2::scoped_connection _applied_block_connection; @@ -427,7 +425,7 @@ namespace graphene { namespace app { /** * @brief The network_broadcast_api class allows broadcasting of transactions. */ - class network_broadcast_api + class network_broadcast_api : public std::enable_shared_from_this { public: network_broadcast_api(application& a);