Fix itf fake peers on demand ordering test #2067

MBoldyrev · 2019-01-31T14:10:18Z

Description of the Change

Rewrote the test...

Benefits

...to fix an unidentified bug and to improve the test logic.

test/integration/acceptance/fake_peer_example_test.cpp

l4l · 2019-01-31T21:54:56Z

test/integration/acceptance/fake_peer_example_test.cpp

+                [&proposal](iroha::expected::Value<std::unique_ptr<
+                                shared_model::interface::Proposal>> &v) {
+                  auto p1 =
+                      std::static_pointer_cast<shared_model::proto::Proposal>(


D: may you a bit elaborate what is happening here?

That was a debugging code I forgot to remove, sorry.

test/integration/acceptance/fake_peer_example_test.cpp

lebdron · 2019-02-26T14:07:55Z

test/integration/acceptance/fake_peer_example_test.cpp

        const auto valid_hash =
            valid_block_storage->getBlockByHeight(committed_block->height())
                ->hash()
                .hex();
        const auto commited_hash = committed_block->hash().hex();
-        if (commited_hash != valid_hash) {
+        if (commited_hash == valid_hash) {
          ASSERT_EQ(commited_hash, valid_hash) << "Wrong block got committed!";


Looks like conditional in previous line should be removed, otherwise ASSERT_EQ will always pass.

lebdron · 2019-02-26T14:19:37Z

test/integration/acceptance/fake_peer_example_test.cpp

+                                shared_model::interface::Proposal>> &v) {
+                  // casting std::shared_ptr<shared_model::interface::Proposal>
+                  // to std::shared_ptr<const shared_model::proto::Proposal>
+                  proposal = std::const_pointer_cast<


Suggested change

proposal = std::const_pointer_cast<

proposal = std::static_pointer_cast<

Because static cast allows to add const.

Signed-off-by: Mikhail Boldyrev <[email protected]>

This reverts commit 56283d5. Signed-off-by: Mikhail Boldyrev <[email protected]>

Signed-off-by: Mikhail Boldyrev <[email protected]>

lebdron

Code in fake_peer::Behaviour looks fragile with this calls captured in observable callback. Maybe we should consider looking at class diagram in order to simplify FakePeer and shared_ptr - weak_ptr relations in the future.

lebdron · 2019-03-31T06:11:13Z

test/framework/integration_framework/fake_peer/behaviour/behaviour.cpp

@@ -25,25 +25,37 @@ namespace integration_framework {
      subscriptions_.emplace_back(
          getFakePeer().getMstStatesObservable().subscribe(
              [this](const auto &message) {
-                this->processMstMessage(message);
+                if (auto alive = fake_peer_wptr_.lock()) {


Will this object outlive the observable? If there are no such guarantees, probably fake_peer_wptr_ has to be captured by value.
Same below.

great catch, thanks!

Signed-off-by: Mikhail Boldyrev <[email protected]>

luckychess

CI tells that fake_peer_example_test still fails. Looks suspicious.

luckychess · 2019-04-02T15:35:35Z

test/framework/integration_framework/fake_peer/fake_peer.cpp

@@ -124,6 +124,12 @@ namespace integration_framework {
      yac_transport_->subscribe(yac_network_notifier_);
    }

+    FakePeer::~FakePeer() {
+      if (behaviour_) {
+        behaviour_->absolve();


As I can see Behaviour class calls absolve by itself in its destructor. Could you explain why we should do it manually here?

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.

But it will be called later anyway, what's the problem?

If we do not call absolve() here, a behaviour (if it overlives the peer) will try to lock its weak pointer and will not succeed. This line absolves its duties on fake peer exit.

luckychess · 2019-04-02T15:55:50Z

test/framework/integration_framework/fake_peer/proposal_storage.cpp

+      std::atomic_store_explicit(
+          &default_provider_,
+          std::make_shared<DefaultProvider>(std::move(provider)),
+          std::memory_order_release);


I would suggest to add some comments about why memory order here and below is important.

It is a general case of release-acquire model with nothing specific.

Ok, what will happen if I omit memory_order arguments?

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.

luckychess · 2019-04-02T15:56:46Z

test/integration/acceptance/fake_peer_example_test.cpp

-static constexpr std::chrono::seconds kMstStateWaitingTime(10);
-static constexpr std::chrono::seconds kSynchronizerWaitingTime(10);
-static constexpr std::chrono::seconds kProposalWaitingTime(1);
+static constexpr std::chrono::seconds kMstStateWaitingTime(20);


Pretty suspicious change.

Since rxcpp timeout works well now, we can loosen the time limits, that were previously lowered to reduce the test execution time.

muratovv · 2019-04-03T07:33:22Z

test/framework/integration_framework/fake_peer/behaviour/behaviour.cpp

-                this->processMstMessage(message);
+              [weak_this,
+               weak_fake_peer = fake_peer_wptr_](const auto &message) {
+                auto me_alive = weak_this.lock();


Code of locking and checking is duplicated 5 times. I think better to refactor it.

muratovv · 2019-04-03T07:45:17Z

test/framework/integration_framework/iroha_instance.cpp

@@ -16,6 +16,8 @@

 using namespace std::chrono_literals;

+static constexpr std::chrono::milliseconds kMstEmissionPeriod = 100ms;


I guess better to move default constants from itf to a separate header.

Signed-off-by: Mikhail Boldyrev <[email protected]>

* new logger integration to itf fake peers * updating batches propagation without round * Fix itf fake peers on demand ordering test (#2067) * fix the fake peer ordering test * reworked ordering test without sync * reworked synchronization test without sync * reworked mst test without sync * absolve behaviour on FakePeer destruction * fixed fake peer lifetime in behaviour subscriptions * use the new ProposalStorage iface to add tx * reduce timeouts * reduce MST delay * state alive check in behaviour subscriptions * removed explicit memory order * Locker class in Behaviour * fixated proposal storage in fake peer * synchronized subjects on_next() Signed-off-by: Mikhail Boldyrev <[email protected]>

* fix the fake peer ordering test * reworked ordering test without sync * reworked synchronization test without sync * reworked mst test without sync * absolve behaviour on FakePeer destruction * fixed fake peer lifetime in behaviour subscriptions * use the new ProposalStorage iface to add tx * reduce timeouts * reduce MST delay * state alive check in behaviour subscriptions * removed explicit memory order * Locker class in Behaviour * fixated proposal storage in fake peer * synchronized subjects on_next() Signed-off-by: Mikhail Boldyrev <[email protected]>

MBoldyrev added needs-review pr awaits review from maintainers bug bug/defect that was reproduced by maintainers tests pr aimed at the code coverage increase or test refactorings labels Jan 31, 2019

MBoldyrev self-assigned this Jan 31, 2019

MBoldyrev requested review from lebdron and Akvinikym January 31, 2019 14:10

l4l reviewed Jan 31, 2019

View reviewed changes

test/integration/acceptance/fake_peer_example_test.cpp Outdated Show resolved Hide resolved

l4l reviewed Jan 31, 2019

View reviewed changes

test/integration/acceptance/fake_peer_example_test.cpp Outdated Show resolved Hide resolved

MBoldyrev requested review from muratovv and removed request for Akvinikym February 4, 2019 07:10

lebdron approved these changes Feb 26, 2019

View reviewed changes

MBoldyrev force-pushed the fix/itf-fake-peer-on-demand-ordering branch from b459159 to 7417957 Compare March 18, 2019 09:43

MBoldyrev changed the base branch from trunk/itf-fake-peer to feature/itf-fake-peer-merge-develop-2019-03-18 March 18, 2019 09:43

MBoldyrev added 10 commits March 18, 2019 12:46

fix the fake peer ordering test

4e68277

Signed-off-by: Mikhail Boldyrev <[email protected]>

pr fix

1a34683

Signed-off-by: Mikhail Boldyrev <[email protected]>

reworked ordering test without sync

2e3440f

Signed-off-by: Mikhail Boldyrev <[email protected]>

reworked synchronization test without sync

893035b

Signed-off-by: Mikhail Boldyrev <[email protected]>

reworked mst test without sync

cbeb498

Signed-off-by: Mikhail Boldyrev <[email protected]>

removed unneeded headers

a61fde1

Signed-off-by: Mikhail Boldyrev <[email protected]>

rm unused var

58bf6b2

Signed-off-by: Mikhail Boldyrev <[email protected]>

pr fixes

d98de05

Signed-off-by: Mikhail Boldyrev <[email protected]>

absolve behaviour on FakePeer destruction

ea19cbf

Signed-off-by: Mikhail Boldyrev <[email protected]>

keep fake peer pointer in subscribers

56283d5

Signed-off-by: Mikhail Boldyrev <[email protected]>

MBoldyrev force-pushed the fix/itf-fake-peer-on-demand-ordering branch from 7417957 to 56283d5 Compare March 18, 2019 09:48

MBoldyrev added 2 commits March 27, 2019 15:31

Revert "keep fake peer pointer in subscribers"

c404910

This reverts commit 56283d5. Signed-off-by: Mikhail Boldyrev <[email protected]>

fixed fake peer lifetime in behaviour subscriptions

b3d01dc

Signed-off-by: Mikhail Boldyrev <[email protected]>

MBoldyrev requested a review from lebdron March 27, 2019 12:43

MBoldyrev added 2 commits March 27, 2019 15:55

use the new ProposalStorage iface to add tx

e779ebe

Signed-off-by: Mikhail Boldyrev <[email protected]>

reduce timeouts

9e2d35a

Signed-off-by: Mikhail Boldyrev <[email protected]>

MBoldyrev added 2 commits March 27, 2019 16:19

fixed timeouts

54c7f3d

Signed-off-by: Mikhail Boldyrev <[email protected]>

reduce MST delay

7ba327c

Signed-off-by: Mikhail Boldyrev <[email protected]>

MBoldyrev force-pushed the fix/itf-fake-peer-on-demand-ordering branch from 6ce87e3 to 7ba327c Compare March 27, 2019 14:07

lebdron approved these changes Mar 31, 2019

View reviewed changes

state alive check in behaviour subscriptions

3c61bdf

Signed-off-by: Mikhail Boldyrev <[email protected]>

lebdron requested a review from luckychess April 2, 2019 10:44

luckychess reviewed Apr 2, 2019

View reviewed changes

muratovv approved these changes Apr 3, 2019

View reviewed changes

MBoldyrev added 5 commits April 4, 2019 08:55

removed explicit memory order

1080eca

Signed-off-by: Mikhail Boldyrev <[email protected]>

Locker class in Behaviour

a8faed4

Signed-off-by: Mikhail Boldyrev <[email protected]>

fixated proposal storage in fake peer

e537546

Signed-off-by: Mikhail Boldyrev <[email protected]>

synchronized subjects on_next()

517fe7f

Signed-off-by: Mikhail Boldyrev <[email protected]>

add this-> to lambda body

3dc62ea

Signed-off-by: Mikhail Boldyrev <[email protected]>

MBoldyrev merged commit ada2f31 into feature/itf-fake-peer-merge-develop-2019-03-18 Apr 4, 2019

MBoldyrev deleted the fix/itf-fake-peer-on-demand-ordering branch April 4, 2019 16:14

lebdron mentioned this pull request May 6, 2019

Hyperledger Iroha v1.0 hyperledger-iroha/iroha#61

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix itf fake peers on demand ordering test #2067

Fix itf fake peers on demand ordering test #2067

MBoldyrev commented Jan 31, 2019

l4l Jan 31, 2019

MBoldyrev Feb 1, 2019

lebdron Feb 26, 2019

lebdron Feb 26, 2019

lebdron left a comment

lebdron Mar 31, 2019

MBoldyrev Mar 31, 2019

luckychess left a comment

luckychess Apr 2, 2019

MBoldyrev Apr 3, 2019

luckychess Apr 3, 2019

MBoldyrev Apr 3, 2019

luckychess Apr 2, 2019

MBoldyrev Apr 3, 2019

luckychess Apr 3, 2019

MBoldyrev Apr 3, 2019

luckychess Apr 2, 2019

MBoldyrev Apr 3, 2019

muratovv Apr 3, 2019

muratovv Apr 3, 2019

	proposal = std::const_pointer_cast<
	proposal = std::static_pointer_cast<

		@@ -16,6 +16,8 @@

		using namespace std::chrono_literals;

		static constexpr std::chrono::milliseconds kMstEmissionPeriod = 100ms;

Fix itf fake peers on demand ordering test #2067

Fix itf fake peers on demand ordering test #2067

Conversation

MBoldyrev commented Jan 31, 2019

Description of the Change

Benefits

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lebdron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luckychess left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment