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

Include heartbeat rate in the hypercore-protocol handshake #18

Open
pfrazee opened this issue Mar 26, 2018 · 3 comments
Open

Include heartbeat rate in the hypercore-protocol handshake #18

pfrazee opened this issue Mar 26, 2018 · 3 comments

Comments

@pfrazee
Copy link
Contributor

pfrazee commented Mar 26, 2018

It turns out that peers need to agree on the heartbeat rate, otherwise they'll incorrectly detect a dead connection. For instance, if peer A expects a rate of 1 every 1s and peer B expects a rate of 1 every 5s, then peer A will believe the connection is dead because too many heartbeat "ticks" will pass with nothing from peer B.

This means the peers need to agree on heartbeat rates in order to maintain good connections. We have two options:

  • Option 1, hardcode the heartbeat rate in the spec
  • Option 2, include a heartbeat negotiation in the handshake

@mafintosh suggested option 2. In that case, we need to decide what rate to use if the peers suggest different values. Perhaps an average, or a lower limit, or an upper limit. Also some limit range should be hardcoded, ie 5s < x < 120s.

If we want to move forward with that, we'll need to update the handshake schema

@bnewbold
Copy link
Contributor

bnewbold commented Apr 2, 2018

Couldn't we make the heartbeak a SYN/ACK thing, where the remote is expected to reply to (acknowledge) any heartbeat? If that were the case, then the effective heartbeat period would be the minimum of the two peers' periods, without any other communication necessary.

Having a configurable (negotiated?) heartbeat rate is adding a piece of "state" to the connection (what is the current rate? can it be updated over time? are there min/max allowed values? option#2 would be immutable), and I think it's nice to avoid extra state when possible. On the other hand, controlling this parameter may end up being important for "supernodes" or "observers" that are connected to many peers for long periods of time. If connected to 2k peers, and the heartbeat is once every 5 seconds, that's 400 hearbeats a second, which is manageable but becoming non-trivial.

@mafintosh
Copy link

@bnewbold good idea. I'll cook something up

@bnewbold
Copy link
Contributor

bnewbold commented Apr 4, 2018

To carry over some conversation from a WG meeting:

Heartbeats and Keep-Alives are already implemented by some reliable transports (eg, TCP), so it could be considered redundant to do this as the application layer. On the other hand, doing it at the application layer makes configuration and behavior consistent across transports, and potentially supports reliable stream transports (or platforms) which do not expose timeout and keep-alive configuration to application code. Other protocols (eg, bittorrent) implement heartbeats/keepalive at the application layer.

Some concern about bandwidth consumption was expressed, as well as concern about battery and CPU consumption for multiple clients. IMHO it should be possible to bundle keep alive behavior in clients (eg, set timers to a coarse global granularity instead of per-connection timers), and to set relatively long (or exponentially growing?) periods to reduce impact. Additionally, "servers" (services and super-nodes) could be configured to skip or have extremely long (5+ minute) hearbeat periods to reduce traffic on "clients" (browsers or mobile devices).

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

No branches or pull requests

3 participants