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

feat: track node connection state #1719

Merged
merged 3 commits into from
Nov 27, 2023
Merged

Conversation

adklempner
Copy link
Member

@adklempner adklempner commented Nov 10, 2023

Problem

There is currently no easy way to check if a waku node is connected to any other peers.

Solution

Adds a function isConnected to the Waku interface which returns a boolean indicating whether or not the node has any active connections.

The node initially starts offline. Once the first peer:connect event is detected by the ConnectionManager, it will now be considered offline. If the ConnectionManager detects a peer:disconnect event, and there are no timers for either relay or ping in the KeepAliveManager, then the node will be considered offline.

Users can either call isConnected on the Waku node instance or add an event listener to the connection manager for waku:online or waku:offline events

Notes

Copy link

github-actions bot commented Nov 10, 2023

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
Waku core 78.1 KB (+0.2% 🔺) 1.6 s (+0.2% 🔺) 1.7 s (+22.41% 🔺) 3.3 s
Waku Simple Light Node 240.62 KB (+0.07% 🔺) 4.9 s (+0.07% 🔺) 3 s (+11.7% 🔺) 7.9 s
ECIES encryption 72.91 KB (+0.07% 🔺) 1.5 s (+0.07% 🔺) 2.1 s (-7.15% 🔽) 3.6 s
Symmetric encryption 72.92 KB (+0.07% 🔺) 1.5 s (+0.07% 🔺) 2.1 s (+6.4% 🔺) 3.6 s
DNS discovery 120.85 KB (0%) 2.5 s (0%) 2.5 s (+7.59% 🔺) 4.9 s
Privacy preserving protocols 125.66 KB (+0.03% 🔺) 2.6 s (+0.03% 🔺) 2.2 s (+5.97% 🔺) 4.7 s
Light protocols 75.71 KB (+0.01% 🔺) 1.6 s (+0.01% 🔺) 1.6 s (-24.95% 🔽) 3.1 s
History retrieval protocols 74.64 KB (+0.04% 🔺) 1.5 s (+0.04% 🔺) 1.5 s (-20.98% 🔽) 3 s
Deterministic Message Hashing 5.65 KB (0%) 113 ms (0%) 145 ms (-63.43% 🔽) 258 ms

@adklempner adklempner force-pushed the adklempner/node-connection-state branch 2 times, most recently from a8d198f to 4c0a743 Compare November 16, 2023 03:50
})();
},
"peer:disconnect": () => {
return (evt: CustomEvent<PeerId>): void => {
"peer:disconnect": (evt: CustomEvent<PeerId>): void => {
Copy link
Member Author

Choose a reason for hiding this comment

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

When I tested locally, this event handler was not being called at all until I changed it to use the same pattern as the two above

@adklempner adklempner force-pushed the adklempner/node-connection-state branch from 4c0a743 to 1bebcbc Compare November 16, 2023 04:18
@adklempner adklempner marked this pull request as ready for review November 16, 2023 04:18
@adklempner adklempner requested a review from a team as a code owner November 16, 2023 04:18
@adklempner adklempner force-pushed the adklempner/node-connection-state branch from 1bebcbc to 2e34b07 Compare November 16, 2023 08:34
@adklempner adklempner force-pushed the adklempner/node-connection-state branch from 2e34b07 to b1e7a63 Compare November 16, 2023 22:19
@adklempner adklempner force-pushed the adklempner/node-connection-state branch from b1e7a63 to 73f4126 Compare November 21, 2023 05:16
Copy link
Collaborator

@danisharora099 danisharora099 left a comment

Choose a reason for hiding this comment

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

looks great!

Comment on lines +51 to +71
private toggleOnline(): void {
if (!this.online) {
this.online = true;
this.dispatchEvent(
new CustomEvent<boolean>(EConnectionStateEvents.CONNECTION_STATUS, {
detail: this.online
})
);
}
}

private toggleOffline(): void {
if (this.online && this.libp2p.getConnections().length == 0) {
this.online = false;
this.dispatchEvent(
new CustomEvent<boolean>(EConnectionStateEvents.CONNECTION_STATUS, {
detail: this.online
})
);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: maybe this particular dispatch event can be written as a private function for the class: dispatchConnectionStatus() since it's being reused twice.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a usual concern with these type of functions. I like current one as it is explicitly mentions when it becomes online and when offline.

@adklempner can follow up if thinks it is necessary

@weboko weboko merged commit 1d0e2ac into master Nov 27, 2023
@weboko weboko deleted the adklempner/node-connection-state branch November 27, 2023 11:44
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.

feat: node connection state
4 participants