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

Listener is not ready yet (Websocket and pubsub discovery) #2902

Open
sfroment opened this issue Dec 25, 2024 · 0 comments · May be fixed by #2903
Open

Listener is not ready yet (Websocket and pubsub discovery) #2902

sfroment opened this issue Dec 25, 2024 · 0 comments · May be fixed by #2903
Labels
need/triage Needs initial labeling and prioritization

Comments

@sfroment
Copy link

sfroment commented Dec 25, 2024

Hello 👋

  • Platform: MacOS (but the bug will be present on every platform)

Severity: Medium/High

Description: So while working on Topology ts-drp. I encounter this

/node_modules/.pnpm/@[email protected]/node_modules/@libp2p/websockets/src/listener.ts:347
      throw new Error('Listener is not ready yet')
            ^


Error: Listener is not ready yet
    at WebSocketListener.getAddrs (/node_modules/.pnpm/@[email protected]/node_modules/@libp2p/websockets/src/listener.ts:347:13)
    at DefaultTransportManager.getAddrs (/node_modules/.pnpm/[email protected]/node_modules/libp2p/src/transport-manager.ts:136:40)
    at AddressManager.getAddressesWithMetadata (/node_modules/.pnpm/[email protected]/node_modules/libp2p/src/address-manager/index.ts:358:40)
    at AddressManager.getAddresses (/node_modules/.pnpm/[email protected]/node_modules/libp2p/src/address-manager/index.ts:304:29)
    at @libp2p/pubsub-peer-discovery._broadcast (/node_modules/.pnpm/@[email protected]/node_modules/@libp2p/pubsub-peer-discovery/src/index.ts:247:44)
    at @libp2p/pubsub-peer-discovery.afterStart (/node_modules/.pnpm/@[email protected]/node_modules/@libp2p/pubsub-peer-discovery/src/index.ts:203:5)
    at <anonymous> (/node_modules/.pnpm/[email protected]/node_modules/libp2p/src/components.ts:80:6)
    at Array.map (<anonymous>)
    at Proxy._invokeStartableMethod (/node_modules/.pnpm/[email protected]/node_modules/libp2p/src/components.ts:77:10)
    at Proxy.afterStart (/node_modules/.pnpm/[email protected]/node_modules/libp2p/src/components.ts:96:9)

The usage of pubsubdiscovery is broken if you only use the ws transport. As not the pubsub won't be able to call this.addressManager.getAddresses() in the afterStart like it does right now.
The problem is both the late init of the WS server, and the fact that all the after start are called at the same time in components.ts

  private async _invokeStartableMethod (methodName: 'beforeStart' | 'start' | 'afterStart' | 'beforeStop' | 'stop' | 'afterStop'): Promise<void> {
    await Promise.all(
      Object.values(this.components)
        .filter(obj => isStartable(obj))
        .map(async (startable: Startable) => {
          await startable[methodName]?.()
        })
    )
  }

Steps to reproduce the error: Just add this unit test in packages/transport-websockets/test/node.ts

class TestPeerDiscovery extends TypedEventEmitter<PeerDiscoveryEvents> implements PeerDiscovery, Startable {
  private readonly addressManager: AddressManager

  constructor (components: {
    addressManager: AddressManager
  }) {
    super()
    this.addressManager = components.addressManager
  }

  async start (): Promise<void> {
  }

  async stop (): Promise<void> { }

  afterStart (): void {
    this.addressManager.getAddresses()
  }

  readonly [Symbol.toStringTag] = '@libp2p/test-peer-discovery'
}

describe('discovery-websockets', () => {
  it('should discover peers over websockets', async () => {
    const libp2p = await createLibp2p({
      addresses: {
        listen: [
          '/ip4/0.0.0.0/tcp/0/ws'
        ]
      },
      peerDiscovery: [(component: {
        addressManager: AddressManager
      }) => new TestPeerDiscovery(component)],
      transports: [
        webSockets()
      ]
    })

    await libp2p.start()
  })
})

I've made a fix but I don't fix this is the one you'd want to add in the repo.
So:

  • we can change the way the conn manager is listening and make the listen in start ?
  • change the way components.ts will call the afterStart so it's not in a promise all but rather in an "ordered" way
  • change the way the WS server is construct/inited ?

What ever solution you'll decide to implement could you please ping me so I can see the way you'll do it 🙏
Nevertheless here's my shot at it: #2903

@sfroment sfroment added the need/triage Needs initial labeling and prioritization label Dec 25, 2024
@sfroment sfroment linked a pull request Dec 25, 2024 that will close this issue
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need/triage Needs initial labeling and prioritization
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant