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

Resend messages on failure #1705

Merged
merged 15 commits into from Jul 11, 2019

Conversation

Projects
None yet
4 participants
@fizyk20
Copy link
Contributor

commented Jul 4, 2019

Closes #1684.

This introduces a new event in mock-quic-p2p, which is not yet present in production, so for now only the testing configuration compiles correctly.

@jeanphilippeD jeanphilippeD self-requested a review Jul 4, 2019

@jeanphilippeD
Copy link
Contributor

left a comment

Had a look through all change combined.
This looks really good.

2 main thing somewhat related:

  • is closest_known_names doing the right thing in the context of disjoint section.
  • Did we need to change the use of connected_peers as part of this PR? (i.e it could be before or after, or is it needed?).
Show resolved Hide resolved src/chain/chain.rs Outdated
Show resolved Hide resolved src/chain/chain.rs Outdated
Show resolved Hide resolved src/chain/chain.rs Outdated
Show resolved Hide resolved src/mock/quic_p2p/tests.rs
Show resolved Hide resolved src/network_service/mod.rs
Show resolved Hide resolved src/network_service/sending_targets_cache.rs Outdated
Show resolved Hide resolved src/network_service/sending_targets_cache.rs Outdated
Show resolved Hide resolved src/peer_map.rs Outdated
Show resolved Hide resolved src/states/common/base.rs
Show resolved Hide resolved src/states/common/base.rs Outdated

@fizyk20 fizyk20 force-pushed the fizyk20:issue_1684 branch 2 times, most recently from 7c7e66e to 4fd9aa2 Jul 5, 2019

@fizyk20 fizyk20 force-pushed the fizyk20:issue_1684 branch from 4fd9aa2 to 1a7ba07 Jul 5, 2019

@fizyk20 fizyk20 force-pushed the fizyk20:issue_1684 branch from 1a7ba07 to 0b118f1 Jul 5, 2019

Show resolved Hide resolved src/states/common/base.rs Outdated
@jeanphilippeD
Copy link
Contributor

left a comment

Looks great, few minor coments already published.

@fizyk20 fizyk20 force-pushed the fizyk20:issue_1684 branch from d8f3fbb to 8aac755 Jul 5, 2019

@jeanphilippeD
Copy link
Contributor

left a comment

Looks good to me.

@fizyk20 fizyk20 force-pushed the fizyk20:issue_1684 branch from a9e7911 to 21c8210 Jul 8, 2019

Show resolved Hide resolved src/mock/quic_p2p/node.rs Outdated
Show resolved Hide resolved src/mock/quic_p2p/node.rs Outdated

@fizyk20 fizyk20 force-pushed the fizyk20:issue_1684 branch 2 times, most recently from 8355fbc to e467a31 Jul 8, 2019

fizyk20 and others added some commits Jun 20, 2019

feat/mock-quic-p2p: revert temporary "fix"
Revert to sending failure events when message recipient is not connected
to sender.
feat/mock: pretend quic-p2p exposes `SentUserMessage`
Change the mock to expose what we wish the implementation would expose.
We will update the actual implementation once the mock feature is
convenient enough for the features we need in routing.

In this case, we would like for quic-p2p to inform its users once a
message cannot result in an `UnsentUserMessage` anymore.
This will remove the need for routing to keep its own timeouts in a way
or another (probably as a `LruTimeCache`).
fix/test: remove side-effect from lose_proxy_connection
By the way it was constructed, the test `lose_proxy_connection` was
enforcing a specific number of network or events.
This coupling was unrelated to the core logic of this test and made it
brittle.
Relax this coupling by only imposing a lower bound on the number of
state machine steps according to the test logic.

Note that this test could benefit from a more thorough refactoring, but
this particular change limits itself to what's necessary to ensure
resilience to an upcoming change.
feat/chain: update targets method to return potential targets
Chain::targets now returns a list of potential targets sorted by
priority (distance), as well as a number saying how many of these
targets should be used initially. The others will be cached and only
used if one or more of the initial targets fail.
feat/base: use the sending targets cache when sending
This commit actually makes the message sending methods insert the
message into the cache - so that they can be re-sent when a failure
occurs.

@fizyk20 fizyk20 force-pushed the fizyk20:issue_1684 branch from e467a31 to 9a4becc Jul 8, 2019

@jeanphilippeD

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

Looks good.

@fizyk20 fizyk20 requested a review from maqi Jul 9, 2019

@@ -4,7 +4,7 @@ set -x -e
export RUSTFLAGS="-C opt-level=2 -C codegen-units=8"

cargo fmt -- --check
cargo clippy $@ --all-targets
#cargo clippy $@ --all-targets

This comment has been minimized.

Copy link
@maqi

maqi Jul 10, 2019

Member

why this to be commented out?
if does not required any more?
shall it be removed, instead of comment out?

This comment has been minimized.

Copy link
@fizyk20

fizyk20 Jul 10, 2019

Author Contributor

It's temporary - this is because the non-mock code no longer compiles until quic-p2p's API is updated to match what we did here in mock-quic-p2p. Once quic-p2p is updated, this will be restored.

@@ -1091,7 +1091,7 @@ impl Chain {
&self,
dst: &Authority<XorName>,
connected_peers: &[&XorName],
) -> Result<BTreeSet<XorName>, Error> {
) -> Result<(Vec<XorName>, usize), Error> {

This comment has been minimized.

Copy link
@maqi

maqi Jul 10, 2019

Member

better update the comment to explain what this usize about?

Show resolved Hide resolved src/mock/quic_p2p/mod.rs Outdated
Show resolved Hide resolved src/network_service/sending_targets_cache.rs
Show resolved Hide resolved src/network_service/sending_targets_cache.rs
})
.min_by_key(|(_info, num, _state)| *num)
.map(|(info, num, state)| {
*state = TargetState::Sending(num);

This comment has been minimized.

Copy link
@maqi

maqi Jul 10, 2019

Member

Failed is then be updated to Sending ?

This comment has been minimized.

Copy link
@fizyk20

fizyk20 Jul 10, 2019

Author Contributor

The target that is Failed with the lowest number of times is taken and updated to Sending. I can add a comment 👍 It's basically what JP described in issue #1684.

.collect();

if conn_infos.len() < dg_size {
warn!(

This comment has been minimized.

Copy link
@maqi

maqi Jul 10, 2019

Member

Shall this be a log_or_panic, so that we can debug it during test?

This comment has been minimized.

Copy link
@fizyk20

fizyk20 Jul 10, 2019

Author Contributor

I'm not sure if it's worth panicking here even during tests - if this happening always indicates a bug. Not totally against it, though. @jeanphilippeD what is your opinion?

This comment has been minimized.

Copy link
@jeanphilippeD

jeanphilippeD Jul 10, 2019

Contributor

I had this message in the follow up PR

WARN 14:47:44.561105308 [routing::states::common::base base.rs:325] Elder(ef4ac7..(111)) Less than dg_size valid targets! dg_size = 1; targets = Filter { iter: Iter([]) }; msg = Hop(HopMessage { content: SignedRoutingMessage { content: RoutingMessage { src: PrefixSection(prefix: Prefix(111)), dst: PrefixSection(prefix: Prefix(110)), content: Merge(64f9c4..2e1dcf) }, sending nodes: Some(SectionInfo(prefix: Prefix(111), members: {PublicId(name: ef4ac7..), PublicId(name: f32c59..), PublicId(name: f56f74..), PublicId(name: f853ad..), PublicId(name: ffa334..)}, prev_hash_len: 1, version: 19)), signatures: ProofSet { [PublicId(name: ef4ac7..), PublicId(name: f56f74..), PublicId(name: f853ad..), PublicId(name: ffa334..)] }, proving_sections: [] }, signature: .. })

With the following seed: Some([2044325391, 837965916, 820907250, 664787039])

So if it is always a bug we have a bug. But also, if we panic here we would not be able to push it.

This comment has been minimized.

Copy link
@fizyk20

fizyk20 Jul 10, 2019

Author Contributor

Maybe we should investigate what caused this message with that seed and see if it's a legitimate situation, or a bug?

This comment has been minimized.

Copy link
@jeanphilippeD

jeanphilippeD Jul 10, 2019

Contributor

I'll create an issue once we merge both yours and mine.
But since it is related to merge I'd prefer avoiding looking at it too deeply for now.

@maqi

maqi approved these changes Jul 11, 2019

@jeanphilippeD jeanphilippeD merged commit d7539ac into maidsafe:fleming Jul 11, 2019

2 checks passed

Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@fizyk20 fizyk20 deleted the fizyk20:issue_1684 branch Jul 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.