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: node connection state #1666

Closed
Tracked by #95
weboko opened this issue Oct 15, 2023 · 12 comments · Fixed by #1719
Closed
Tracked by #95

feat: node connection state #1666

weboko opened this issue Oct 15, 2023 · 12 comments · Fixed by #1719
Assignees
Labels
E:Presentation Readiness See https://github.com/waku-org/pm/issues/95 for details enhancement New feature or request

Comments

@weboko
Copy link
Collaborator

weboko commented Oct 15, 2023

This is a feature request

Problem

It is problematic for novice people to check if browser node is connected to Waku network

Proposed Solutions

Enable users to easily check online/offline status and maybe other things. Good reference could be current browser behavior.

console.log('Initially ' + (window.navigator.onLine ? 'on' : 'off') + 'line');

window.addEventListener('online', () => console.log('Became online'));
window.addEventListener('offline', () => console.log('Became offline'));
@weboko weboko added the enhancement New feature or request label Oct 15, 2023
@weboko weboko self-assigned this Oct 15, 2023
@weboko weboko added the E:Presentation Readiness See https://github.com/waku-org/pm/issues/95 for details label Oct 16, 2023
@danisharora099
Copy link
Collaborator

TODO: we need to when your js-waku node is connected to at least one peer/disconnected from all peers
@danisharora099

@danisharora099 danisharora099 removed their assignment Nov 8, 2023
@weboko
Copy link
Collaborator Author

weboko commented Nov 9, 2023

For first iteration we can go with implementing something like:

get networkState(): string {
  return hasPeers ? NetworkState.Online : NetworkState.Offline;
}

Or we can use event based approach:

const node = createNode();
node.addEventListener("netwrok", (state: online | offline) => { ... });

@adklempner @danisharora099 what do you thing?

@adklempner
Copy link
Member

We may want both. I can imagine scenarios where it's more convenient to poll the network status as versus receiving it as an event, and vice-versa.

@adklempner
Copy link
Member

adklempner commented Nov 10, 2023

Is ConnectionManager the right place for this functionality?

Here's what I'm thinking of adding #1719 :

  private online: boolean = false;

  public isConnected(): boolean {
    return this.online;
  }

  private toggleOnline(): void {
    if (!this.online) {
      this.online = true;
      this.dispatchEvent(new Event("waku:online"));
    }
  }

  private toggleOffline(): void {
    if (this.libp2p.getPeers().length == 0) {
      this.online = false;
      this.dispatchEvent(new Event("waku:offline"));
    }
  }

toggleOnline would be called at the end of the existing handler for peer:connected, and toggleOffline would be called at the end of the existing handler for peer:disconnected

This assumes that having a single peer is enough to be considered "online". In the future we may change it if need be (e.g. if node is setup to use autosharding, it should be connected to at least one peer for each shard to be considered online)

What I'm not sure about is a clean way to pass an event listener for the newly dispatched events (waku:online and waku:offline) from WakuNode to the ConnectionManager.

@adklempner adklempner self-assigned this Nov 13, 2023
@weboko
Copy link
Collaborator Author

weboko commented Nov 13, 2023

Is ConnectionManager the right place for this functionality?

I think for functionality but exposed it should be on the root level as we'd like to have it on the surface.

Overall to me it seems as good approach, I would also use these events.

@danisharora099 what do you think?

@adklempner
Copy link
Member

I changed the way toggleOffline determines if there are any peers connected, because the array returned by this.libp2p.getPeers() was not updating after peer:disconnected events were dispatched. I added a function to KeepAliveManager that determines if there any connections if any relay or ping timers are present: https://github.com/waku-org/js-waku/pull/1719/files#diff-47f53ecbbc63fddf37d939149e70131b808afe18c0ce785c9033e1f1cf5f18c0R114

@danisharora099
Copy link
Collaborator

ConnectionMnaager seems like the right place to include this.
While I see no direct harm with using KeepAliveManager and the pings to gauge connectivity, I'd use libp2p.getConnections() instead.

@adklempner
Copy link
Member

ConnectionMnaager seems like the right place to include this. While I see no direct harm with using KeepAliveManager and the pings to gauge connectivity, I'd use libp2p.getConnections() instead.

@danisharora099 I tried using that function but it always returned an array of length 0 after two peer:connect events were dispatched in the test. Maybe the way I'm setting up the preconditions in the test doesn't match production conditions? Is there something that needs to happen besides dispatching a peer:connect event for the connection to be added to the array returned by libp2p.getConnections()? Here's the test:

it("isConnected should return true after first peer connects", async function () {

@adklempner
Copy link
Member

Weekly Update

  • achieved: modified ConnectionManager to track connection state. exposed through read function and new event
  • next: resolve any remaining feedback

@adklempner
Copy link
Member

ConnectionMnaager seems like the right place to include this. While I see no direct harm with using KeepAliveManager and the pings to gauge connectivity, I'd use libp2p.getConnections() instead.

@danisharora099 I tried using that function but it always returned an array of length 0 after two peer:connect events were dispatched in the test. Maybe the way I'm setting up the preconditions in the test doesn't match production conditions? Is there something that needs to happen besides dispatching a peer:connect event for the connection to be added to the array returned by libp2p.getConnections()? Here's the test:

it("isConnected should return true after first peer connects", async function () {

This could be better tested by actually running one nwaku node and dialing it from the js waku node being tested. This should establish an actual connection and it will appear under libp2p.getConnections()

To disconnect, use this function https://github.com/libp2p/js-libp2p/blob/13a870cbef326a3a3b3c55b886c2109feaa2b628/packages/libp2p/src/libp2p.ts#L301

@danisharora099
Copy link
Collaborator

danisharora099 commented Nov 27, 2023

ConnectionMnaager seems like the right place to include this. While I see no direct harm with using KeepAliveManager and the pings to gauge connectivity, I'd use libp2p.getConnections() instead.

@danisharora099 I tried using that function but it always returned an array of length 0 after two peer:connect events were dispatched in the test. Maybe the way I'm setting up the preconditions in the test doesn't match production conditions? Is there something that needs to happen besides dispatching a peer:connect event for the connection to be added to the array returned by libp2p.getConnections()? Here's the test:

it("isConnected should return true after first peer connects", async function () {

@adklempner
I think the reason it doesn't show up is because while the event is being manually emitted, libp2p doesn't actually add it to the connection list.
I would suggest refactoring this test to actually have service node interop, and dial an actual node and check the connections against that.

@danisharora099
Copy link
Collaborator

this issue is resolved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E:Presentation Readiness See https://github.com/waku-org/pm/issues/95 for details enhancement New feature or request
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants