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

Fix keepalive for connected peers #588

Merged
merged 2 commits into from
Jun 2, 2021
Merged

Fix keepalive for connected peers #588

merged 2 commits into from
Jun 2, 2021

Conversation

jm-clius
Copy link
Contributor

@jm-clius jm-clius commented Jun 1, 2021

This PR fixes #586

Description

It does so by introducing a simple (and optional) ping-like protocol for nim-waku that periodically sends an empty message to connected peers. This a temporary workaround until libp2p ping protocol has been implemented, as set out in vacp2p/nim-libp2p#567. Ping protocol is currently prioritised on the nim-libp2p side, but will likely not be finished before the planned testnet for nim-waku. Waku v2 dogfooding has also commenced and some keepalive mechanism is necessary to maintain stable fleet connections and bridge toy-chat messages to Discord.

How is this different from the libp2p ping protocol?

It is simpler in at least two ways:

  • the keepalive messages do not carry any payload (as opposed to 32-byte ping messages), and
  • it does not provide or expect any response. It is therefore simply a best effort to ensure there is minimal activity on open connections.

Limitations of this approach:

  • it is temporary, though implemented in such a way that it can easily be replaced by ping protocol, once available.
  • it is not interoperable with non-nim implemenations of Waku. Inter-client keepalive will likely only be available once ping protocol is implemented.

@jm-clius jm-clius marked this pull request as ready for review June 1, 2021 19:02
@jm-clius jm-clius requested review from oskarth, staheri14 and EbubeUd June 1, 2021 19:02
Copy link
Contributor

@staheri14 staheri14 left a comment

Choose a reason for hiding this comment

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

Looks good!

Copy link
Contributor

@oskarth oskarth left a comment

Choose a reason for hiding this comment

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

Hm, at what point does it make sense to just do the PING protocol? Any ETA on it? EDIT: I see issue was just assigned

@D4nte
Copy link
Contributor

D4nte commented Jun 2, 2021

Are you using a content topic or are messages fully empty?

@jm-clius
Copy link
Contributor Author

jm-clius commented Jun 2, 2021

Hm, at what point does it make sense to just do the PING protocol? Any ETA on it? EDIT: I see issue was just assigned

Yeah, discussed with nim-libp2p and it's now prioritised on their side, but I'd like us to be unblocked to continue with testnet/dogfooding. This is an intermediate fix necessary to get the fleets and bridge in a working condition again.

@jm-clius
Copy link
Contributor Author

jm-clius commented Jun 2, 2021

Are you using a content topic or are messages fully empty?

Messages are completely empty protobufs, yes. It has no relation to either the PubSub/Waku domains, so contains no reference to content topics or similar. It simply keeps the connection between two switches open.

@jm-clius jm-clius merged commit 2b571e2 into master Jun 2, 2021
@jm-clius jm-clius deleted the fix/keep-alive-fix branch June 2, 2021 07:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: keep-alive failing when mesh is full
4 participants