-
Notifications
You must be signed in to change notification settings - Fork 295
Fix itf fake peers on demand ordering test #2067
Changes from 16 commits
4e68277
1a34683
2e3440f
893035b
cbeb498
a61fde1
58bf6b2
d98de05
ea19cbf
56283d5
c404910
b3d01dc
e779ebe
9e2d35a
54c7f3d
7ba327c
3c61bdf
1080eca
a8faed4
e537546
517fe7f
3dc62ea
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -124,6 +124,12 @@ namespace integration_framework { | |
yac_transport_->subscribe(yac_network_notifier_); | ||
} | ||
|
||
FakePeer::~FakePeer() { | ||
if (behaviour_) { | ||
behaviour_->absolve(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As I can see There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We do it to ensure it will stop its operation on this peer. The destructor may not be called if the behaviour is managed by a shared pointer stored elsewhere. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But it will be called later anyway, what's the problem? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we do not call |
||
} | ||
} | ||
|
||
FakePeer &FakePeer::initialize() { | ||
BOOST_VERIFY_MSG(not initialized_, "Already initialized!"); | ||
// here comes the initialization of members requiring shared_from_this() | ||
|
@@ -381,7 +387,7 @@ namespace integration_framework { | |
|
||
boost::optional<std::shared_ptr<const shared_model::interface::Proposal>> | ||
FakePeer::sendProposalRequest(iroha::consensus::Round round, | ||
std::chrono::milliseconds timeout) { | ||
std::chrono::milliseconds timeout) const { | ||
auto on_demand_os_transport = | ||
iroha::ordering::transport::OnDemandOsClientGrpcFactory( | ||
async_call_, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ | |
|
||
#include "framework/integration_framework/fake_peer/proposal_storage.hpp" | ||
|
||
#include <atomic> | ||
#include <mutex> | ||
|
||
#include <boost/range/adaptor/indirected.hpp> | ||
|
@@ -18,10 +19,11 @@ namespace integration_framework { | |
namespace fake_peer { | ||
|
||
ProposalStorage::ProposalStorage() | ||
: default_provider_([](auto &) { return boost::none; }), | ||
proposal_factory_( | ||
: proposal_factory_( | ||
std::make_unique<shared_model::proto::ProtoProposalFactory< | ||
shared_model::validation::DefaultProposalValidator>>()) {} | ||
shared_model::validation::DefaultProposalValidator>>()) { | ||
setDefaultProvider([](auto &) { return boost::none; }); | ||
} | ||
|
||
OrderingProposalRequestResult ProposalStorage::getProposal( | ||
const Round &round) { | ||
|
@@ -46,7 +48,7 @@ namespace integration_framework { | |
} | ||
|
||
// finally, use the defualt | ||
return default_provider_(round); | ||
return getDefaultProposal(round); | ||
} | ||
|
||
ProposalStorage &ProposalStorage::storeProposal( | ||
|
@@ -114,5 +116,23 @@ namespace integration_framework { | |
|
||
} | ||
|
||
ProposalStorage &ProposalStorage::setDefaultProvider(DefaultProvider provider) { | ||
std::atomic_store_explicit( | ||
&default_provider_, | ||
std::make_shared<DefaultProvider>(std::move(provider)), | ||
std::memory_order_release); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would suggest to add some comments about why memory order here and below is important. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is a general case of release-acquire model with nothing specific. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, what will happen if I omit memory_order arguments? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are right that this optimization is excessive in tests. I believe no noticeable overhead will be added if the default memory order is restored. |
||
return *this; | ||
} | ||
|
||
OrderingProposalRequestResult ProposalStorage::getDefaultProposal( | ||
const Round &round) const { | ||
auto default_provider = std::atomic_load_explicit( | ||
&default_provider_, std::memory_order_acquire); | ||
if (default_provider) { | ||
return default_provider->operator()(round); | ||
} | ||
return {}; | ||
} | ||
|
||
} // namespace fake_peer | ||
} // namespace integration_framework |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,8 @@ | |
|
||
using namespace std::chrono_literals; | ||
|
||
static constexpr std::chrono::milliseconds kMstEmissionPeriod = 100ms; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess better to move default constants from itf to a separate header. |
||
|
||
namespace integration_framework { | ||
|
||
IrohaInstance::IrohaInstance(bool mst_support, | ||
|
@@ -39,7 +41,12 @@ namespace integration_framework { | |
// amount of minutes in a day | ||
mst_expiration_time_(std::chrono::minutes(24 * 60)), | ||
opt_mst_gossip_params_(boost::make_optional( | ||
mst_support, iroha::GossipPropagationStrategyParams{})), | ||
mst_support, | ||
[] { | ||
iroha::GossipPropagationStrategyParams params; | ||
params.emission_period = kMstEmissionPeriod; | ||
return params; | ||
}())), | ||
max_rounds_delay_(0ms), | ||
stale_stream_max_rounds_(2), | ||
irohad_log_manager_(std::move(irohad_log_manager)), | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will
this
object outlive the observable? If there are no such guarantees, probablyfake_peer_wptr_
has to be captured by value.Same below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great catch, thanks!