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

fixes: HocuspocusProviderWebsocket checkConnection ineffective #640

Conversation

grootgordon
Copy link
Contributor

@grootgordon grootgordon commented Jun 28, 2023

In the v1.1.3, onMessage will update lastMessageReceived as follow

onMessage(event: MessageEvent) {
    this.resolveConnectionAttempt()

    this.lastMessageReceived = time.getUnixTime()

    const message = new IncomingMessage(event.data)

    this.emit('message', { event, message })

    new MessageReceiver(message).apply(this)
  }

In the v2.2.0, onMessage not update lastMessageReceived.

//HocuspocusProviderWebsocket.ts
onMessage(event: MessageEvent) {
    this.resolveConnectionAttempt()
}
//HocuspocusProvider.ts
onMessage(event: MessageEvent) {
    const message = new IncomingMessage(event.data)

    const documentName = message.readVarString()

    if (documentName !== this.configuration.name) {
      return // message is meant for another provider
    }

    message.writeVarString(documentName)

    this.emit('message', { event, message: new IncomingMessage(event.data) })

    new MessageReceiver(message).apply(this)
}

If the client switch its network from WIFI to LTE or vice versa,

checkConnection can not able to call webSocket?.close() because of lastMessageReceived = 0 in HocuspocusProviderWebsocket constructor.

checkConnection() {
    // Don’t check the connection when it’s not even established
    if (this.status !== WebSocketStatus.Connected) {
      return
    }

    // Don’t close then connection while waiting for the first message
    if (!this.lastMessageReceived) {
      return
    }

    // Don’t close the connection when a message was received recently
    if (this.configuration.messageReconnectTimeout >= time.getUnixTime() - this.lastMessageReceived) {
      return
    }

    // No message received in a long time, not even your own
    // Awareness updates, which are updated every 15 seconds.
    this.webSocket?.close()
  }

this case can be reproduce in iOS safari or iOS webview(iPhone 15E148 605.1, webkit version:605.1.15), but not easy to find in windows(chrome)、android(chrome/webview)

@janthurau
Copy link
Collaborator

Thanks for the report & even more thanks for fixing it :-)

I think we can just add this to the providerWebsocket, or is there any reason you added it to the provider?

@grootgordon
Copy link
Contributor Author

grootgordon commented Jun 29, 2023

Thanks reply.
Yes, add to the HocuspocusProviderWebsocket.ts more reasonable.
I have modified it.

@janthurau janthurau merged commit 797fc7c into ueberdosis:main Jun 30, 2023
3 checks passed
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.

2 participants