Skip to content

Suggest faster ping in PeerManager::timer_tick_occurred docs #1035

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Aug 9, 2021
Merged

Suggest faster ping in PeerManager::timer_tick_occurred docs #1035

merged 3 commits into from
Aug 9, 2021

Conversation

TheBlueMatt
Copy link
Collaborator

This clarifies the docs for PeerManager::timer_tick_occurred to
note that the call rate is entirely up to the user, and also
suggests a faster ping rate of "once every five to ten seconds"
instead of "every 30 seconds". There isn't a lot of reason to want
to ping less often, and faster ping means we detect disconnects
sooner, which is important.

@TheBlueMatt TheBlueMatt mentioned this pull request Aug 4, 2021
Closed
@codecov
Copy link

codecov bot commented Aug 4, 2021 •
edited
Loading

Codecov Report

Merging #1035 (7a273ed) into main (09e1670) will increase coverage by 1.27%.
The diff coverage is 78.94%.

❗ Current head 7a273ed differs from pull request most recent head 0f1a3b1. Consider uploading reports for the commit 0f1a3b1 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1035      +/-   ##
==========================================
+ Coverage   90.84%   92.11%   +1.27%     
==========================================
  Files          61       61              
  Lines       31534    36953    +5419     
==========================================
+ Hits        28646    34040    +5394     
- Misses       2888     2913      +25
Impacted Files Coverage Δ
lightning/src/ln/peer_handler.rs 45.98% <ø> (ø)
lightning-background-processor/src/lib.rs 94.69% <78.94%> (-0.89%) ⬇️
lightning/src/ln/chanmon_update_fail_tests.rs 97.70% <0.00%> (-0.04%) ⬇️
lightning/src/ln/reorg_tests.rs 100.00% <0.00%> (+0.29%) ⬆️
lightning/src/chain/channelmonitor.rs 91.74% <0.00%> (+0.93%) ⬆️
lightning/src/ln/functional_test_utils.rs 96.27% <0.00%> (+1.09%) ⬆️
lightning/src/ln/functional_tests.rs 98.47% <0.00%> (+1.17%) ⬆️
lightning/src/ln/channel.rs 92.90% <0.00%> (+3.68%) ⬆️
lightning/src/util/events.rs 19.08% <0.00%> (+3.95%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 09e1670...0f1a3b1. Read the comment docs.

@TheBlueMatt TheBlueMatt added this to the 0.0.100 milestone Aug 5, 2021
@TheBlueMatt
Copy link
Collaborator Author

Tagging 0.0.100 cause this is super trivial and a user asked for it.

@TheBlueMatt TheBlueMatt force-pushed the 2021-08-faster-pings branch from 23fc049 to d519609 Compare August 6, 2021 01:36
@TheBlueMatt
Copy link
Collaborator Author

Added an additional commit to handle being in the background for too long. This should resolve issues iOS users have seen where the app is in the background for some time and sockets are actually disconnected but we don't know that as we've been offline for a while. This PR is somewhat higher priority for this reason.

jkczyz
jkczyz approved these changes Aug 9, 2021
View reviewed changes
valentinewallace
valentinewallace approved these changes Aug 9, 2021
View reviewed changes
Copy link
Contributor

@valentinewallace valentinewallace left a comment

Choose a reason for hiding this comment

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

Just nits. LGTM!

lightning-background-processor/src/lib.rs
Comment on lines +172 to +173
peer_manager.timer_tick_occurred();
peer_manager.timer_tick_occurred();
Copy link
Contributor

Choose a reason for hiding this comment

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

It reads to me as odd to disconnect peers this way, rather than explicitly through some disconnect_peer() or so API

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right, to some extent I agree, but at the same times its what has happened - the timer should have fired twice, so that's just what we do. The PeerManager could in the future tweak how it handles timer events and I think this would be more correct.

lightning/src/ln/peer_handler.rs
/// Send pings to each peer and disconnect those which did not respond to the last round of
/// pings.
///
/// This may be called on any timescale you want, however, roughly once every five to ten
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: s/want, however,/want. However,

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Don't think it needs a new sentence here?

@TheBlueMatt TheBlueMatt force-pushed the 2021-08-faster-pings branch 2 times, most recently from 7a273ed to 0c6637e Compare August 9, 2021 18:10
@TheBlueMatt
Suggest faster ping in PeerManager::timer_tick_occurred docs
70653d0 
This clarifies the docs for `PeerManager::timer_tick_occurred` to
note that the call rate is entirely up to the user, and also
suggests a faster ping rate of "once every five to ten seconds"
instead of "every 30 seconds". There isn't a lot of reason to want
to ping less often, and faster ping means we detect disconnects
sooner, which is important.
@TheBlueMatt
Update lightning-background-processor to ping every five seconds
a00eec1 
This updates lightning-background-processor calls to
PeerManager::timer_tick_occurred to match the new suggested rate in
the documentation.
@TheBlueMatt
Handle being asleep for more than double our ping time gracefully
0f1a3b1 
If we've been asleep for double our ping time, for whatever reason,
disconnect all open sockets.
@TheBlueMatt TheBlueMatt force-pushed the 2021-08-faster-pings branch from 0c6637e to 0f1a3b1 Compare August 9, 2021 18:11
@TheBlueMatt
Copy link
Collaborator Author

TheBlueMatt commented Aug 9, 2021 •
edited
Loading

Squashed. Will merge after CI. Full diff from Jeff's ACK is:

$ git diff-tree -U1  de282fa 0f1a3b16
diff --git a/lightning-background-processor/src/lib.rs b/lightning-background-processor/src/lib.rs
index c0af563f..c75c4726 100644
--- a/lightning-background-processor/src/lib.rs
+++ b/lightning-background-processor/src/lib.rs
@@ -164,3 +164,3 @@ impl BackgroundProcessor {
 					// On various platforms, we may be starved of CPU cycles for several reasons.
-					// eg on iOS, if we've been in the background, we will be entirely paused.
+					// E.g. on iOS, if we've been in the background, we will be entirely paused.
 					// Similarly, if we're on a desktop platform and the device has been asleep, we
diff --git a/lightning/src/ln/peer_handler.rs b/lightning/src/ln/peer_handler.rs
index 7a7cfe31..42b26941 100644
--- a/lightning/src/ln/peer_handler.rs
+++ b/lightning/src/ln/peer_handler.rs
@@ -1379,3 +1379,3 @@ impl<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, L: Deref> PeerManager<D
 	/// seconds is preferred. The call rate determines both how often we send a ping to our peers
-	/// and how long they have to respond before we disconnect them.
+	/// and how much time they have to respond before we disconnect them.
 	///
$

@TheBlueMatt TheBlueMatt merged commit 03537cc into lightningdevkit:main Aug 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jkczyz jkczyz jkczyz approved these changes

@valentinewallace valentinewallace valentinewallace approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.0.100
Development

Successfully merging this pull request may close these issues.
3 participants
@TheBlueMatt @jkczyz @valentinewallace