Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Fix tried to send handshake twice#5413

Merged
tomaka merged 5 commits into
paritytech:masterfrom
tomaka:fix-handshake-twice
Mar 30, 2020
Merged

Fix tried to send handshake twice#5413
tomaka merged 5 commits into
paritytech:masterfrom
tomaka:fix-handshake-twice

Conversation

@tomaka
Copy link
Copy Markdown
Contributor

@tomaka tomaka commented Mar 26, 2020 •
edited
Loading

cc #5201

There was a possible state inconsistency in the NotificationsInSubstream.

@tomaka tomaka added the A0-please_review Pull request needs code review. label Mar 26, 2020
tomaka
tomaka commented Mar 26, 2020
View reviewed changes
Comment thread client/network/src/protocol/generic_proto/upgrade/notifications.rs
NotificationsInSubstreamHandshake::NotSent =>
return Poll::Pending,
NotificationsInSubstreamHandshake::NotSent => {
*this.handshake = NotificationsInSubstreamHandshake::NotSent;
Copy link
Copy Markdown
Contributor Author

@tomaka tomaka Mar 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main bugfix is here.
The rest of the PR is only the log statements that I added a bit everywhere in my attempts to understand the issue.

@tomaka tomaka mentioned this pull request Mar 26, 2020
Closed
romanb
romanb reviewed Mar 26, 2020
View reviewed changes
Comment thread client/network/src/protocol/generic_proto/upgrade/notifications.rs Outdated
Comment thread client/network/src/protocol/generic_proto/handler/group.rs Outdated
Comment thread client/network/src/protocol/generic_proto/handler/group.rs Outdated
Comment thread client/network/src/protocol/generic_proto/handler/notif_in.rs
@tomaka tomaka added the B0-silent Changes should not be mentioned in any release notes label Mar 27, 2020
@tomaka tomaka added this to the 2.0 milestone Mar 27, 2020
romanb
romanb approved these changes Mar 30, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@romanb romanb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about the test failure on should_revalidate_across_many_blocks, I think I've seen it before (i.e. I think it is just a flaky test, but don't take my word for it).

@NikVolf
Copy link
Copy Markdown
Contributor

NikVolf commented Mar 30, 2020

Not sure about the test failure on should_revalidate_across_many_blocks, I think I've seen it before (i.e. I think it is just a flaky test, but don't take my word for it).

test was fixed, just merge master @tomaka

@tomaka
Copy link
Copy Markdown
Contributor Author

tomaka commented Mar 30, 2020

#5360 has been merged and triggers this error way more often, so I'm going to merge this fix as soon as the CI passes.

@tomaka
Copy link
Copy Markdown
Contributor Author

tomaka commented Mar 30, 2020 •
edited
Loading

@NikVolf Now it's testing::pool::should_push_watchers_during_maintaince that is failing for what it's worth.
(EDIT: fixed after restarting the CI)

@tomaka tomaka merged commit d21e324 into paritytech:master Mar 30, 2020
@tomaka tomaka deleted the fix-handshake-twice branch March 30, 2020 11:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@romanb romanb romanb approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes

Projects

None yet

Milestone

2.0

Development

Successfully merging this pull request may close these issues.

3 participants

@tomaka @NikVolf @romanb