'peers' event on on SignalingConn emits 'added' peer from peer's close event

disarticulate commented 3 years ago

Checklist

[X] Are you reporting a bug? Use github issues for bug reports and feature requests. For general questions, please use https://discuss.yjs.dev/
[X] Try to report your issue in the correct repository. Yjs consists of many modules. When in doubt, report it to https://github.com/yjs/yjs/issues/

Describe the bug

class WebrtcConn {
   ...
   peer.on('close' => ... announceSignalingInfo(room))
}

generates an announcement which triggers remote peers:

class SignalingConn {
             ...
             case 'publish':
             ...
             room.provider.emit('peers', [{
                removed: [],
                added: [data.from],
                webrtcPeers: Array.from(room.webrtcConns.keys()),
                bcPeers: Array.from(room.bcConns)
              }])

message peers then says this peer was added which is not what's happening, as peer is closing.

To Reproduce Steps to reproduce the behavior:

Open two browsers with peers connected. Listen to room.provider.peers for 'peers' and you'll see the event.removed shows the peerId, but then another events.added shows the peer added again

Expected behavior When a peer closes, it should not generate a peers event that has the peer added.

Screenshots debug:

removed:webrtc:[98ee6477-5037-49ee-bd20-b8f69928dade] WebrtcProvider {…}

y-webrtc.js?9b73:270 announceSignalingInfo Room {…} true
eval @ y-webrtc.js:294
announceSignalingInfo @ y-webrtc.js:289
eval @ y-webrtc.js:245
r.emit @ simplepeer.min.js:6
eval @ simplepeer.min.js:6
y-webrtc.js?9b73:270 announceSignalingInfo Room {…} true
eval @ y-webrtc.js:294
announceSignalingInfo @ y-webrtc.js:289
eval @ y-webrtc.js:245
r.emit @ simplepeer.min.js:6
eval @ simplepeer.min.js:6
y-webrtc.js?9b73:270 announceSignalingInfo Room {…} true
eval @ y-webrtc.js:294
announceSignalingInfo @ y-webrtc.js:289
eval @ y-webrtc.js:245
r.emit @ simplepeer.min.js:6
eval @ simplepeer.min.js:6
y-webrtc.js?9b73:491 {to: "abb23ec9-66aa-4cd3-8f4c-9f9e7c58bc83", from: "98ee6477-5037-49ee-bd20-b8f69928dade", type: "signal", signal: {…}} "98ee6477-5037-49ee-bd20-b8f69928dade"
emitPeerChange @ y-webrtc.js:515
execMessage @ y-webrtc.js:532
Promise.then (async)
eval @ y-webrtc.js:539
eval @ observable.js:78
emit @ observable.js:78
websocket.onmessage @ websocket.js:50
SyncPeers.js?a801:234

added:webrtc:[98ee6477-5037-49ee-bd20-b8f69928dade]

Environment Information Version 89.0.4389.82 (Official Build) snap (64-bit) "y-webrtc": "^10.1.7" "y-protocols": "^1.0.4", "yjs": "^13.5.2"

dmonad commented 3 years ago

When a client disconnects, it automatically tries to reconnnect. Are you sure that the client is disconnected when added:webrtc:[98ee6477-5037-49ee-bd20-b8f69928dade] is fired?

disarticulate commented 3 years ago

Yes.

When I review y-webrtc for the close, (this.peer.on('close', () =>...) the last line is announceSignalingInfo(room) which triggerS:

const announceSignalingInfo = room => {
  signalingConns.forEach(conn => {
    // only subcribe if connection is established, otherwise the conn automatically subscribes to all rooms
    if (conn.connected) {
      conn.send({ type: 'subscribe', topics: [room.name] })
      if (room.webrtcConns.size < room.provider.maxConns) {
        publishSignalingMessage(conn, room, { type: 'announce', from: room.peerId })
      }
    }
  })
}

there's the debugs you're seeing. 'true' is conn.connected. So on close, it's announcing signically messages, even though it's closing.

My naive understanding suggests it shouldn't be announcing or, this shouldn't say 'added' from the message :

            const emitPeerChange = webrtcConns.has(data.from) ? () => {} : () =>
              room.provider.emit('peers', [{
                removed: [],
                added: [data.from],
                webrtcPeers: Array.from(room.webrtcConns.keys()),
                bcPeers: Array.from(room.bcConns)
              }])

dmonad commented 3 years ago

There are three types of connections. 1. There are WebRTC connections. The line you mentioned this.peer.on('close', () =>...) is referring to a webrtc conenction. 2. There are BroadcastChannel connections (connections to other peers in the same browser are not handled via WebRTC). 3. There are Websocket connections to a signaling server. announceSignalingInfo sends a message to the websocket/signaling server. When a WebRTC connection closes unexpectedly, we announce ourselves over the signaling server again to reconnect to the closed connection. After a time, the remote client reconnects (either using 1. WebRTC or via 2. BroadcastChannel) and the peers: {added: [..]} event is fired.

disarticulate commented 3 years ago

alright, what I'm seeing:

    this.peer.on('close', () => {
      this.connected = false
      this.closed = true
      if (room.webrtcConns.has(this.remotePeerId)) {
        room.webrtcConns.delete(this.remotePeerId)
        room.provider.emit('peers', [{
          removed: [this.remotePeerId],
          added: [],
          webrtcPeers: Array.from(room.webrtcConns.keys()),
          bcPeers: Array.from(room.bcConns)
        }])
      }
      checkIsSynced(room)
      this.peer.destroy() ***// should this just be 'this.destroy'***
      log('closed connection to ', logging.BOLD, remotePeerId)
      announceSignalingInfo(room)
    })

the announceSignalingInfo call triggers the announcement and the second client gets a 'peers' notification as the peer being added.

It's possible it's a race condition since I'm just doing it locally between two browsers or it's something stranger with the devserver updating.

dmonad commented 3 years ago

How do you close the connection to the remote client?

disarticulate commented 3 years ago

refresh the browser -> close is triggered

If it was re-logging in, we'd have a different peerId, but the same peer ID shows up on the second client. That's the log that's shown on the second browser window, which I retested just so:

removed:webrtc:[11064215-ac2b-48e2-bfd8-52966676e517] WebrtcProvider {…}

y-webrtc.js?9b73:270 announceSignalingInfo Room {…} true
...
y-webrtc.js?9b73:270 announceSignalingInfo Room {…} true
...
y-webrtc.js?9b73:270 announceSignalingInfo Room {…} true
eval @ y-webrtc.js?9b73:270
announceSignalingInfo @ y-webrtc.js?9b73:265
eval @ y-webrtc.js?9b73:221
r.emit @ simplepeer.min.js?07cd:6
eval @ simplepeer.min.js?07cd:6
error (async)
_setupData @ simplepeer.min.js?07cd:6
p @ simplepeer.min.js?07cd:6
WebrtcConn @ y-webrtc.js?9b73:183
eval @ y-webrtc.js?9b73:501
setIfUndefined @ map.js?4b35:49
execMessage @ y-webrtc.js?9b73:501
Promise.then (async)
eval @ y-webrtc.js?9b73:515
eval @ observable.js?9732:73
emit @ observable.js?9732:73
websocket.onmessage @ websocket.js?49e4:45
y-webrtc.js?9b73:491 {type: "announce", from: "11064215-ac2b-48e2-bfd8-52966676e517"} "11064215-ac2b-48e2-bfd8-52966676e517"
emitPeerChange @ y-webrtc.js?9b73:491
execMessage @ y-webrtc.js?9b73:502
Promise.then (async)
eval @ y-webrtc.js?9b73:515
eval @ observable.js?9732:73
emit @ observable.js?9732:73
websocket.onmessage @ websocket.js?49e4:45
SyncPeers.js?a801:235

added:webrtc:[11064215-ac2b-48e2-bfd8-52966676e517]

WebrtcConn {room: Room, remotePeerId: "11064215-ac2b-48e2-bfd8-52966676e517", closed: false, connected: false, synced: false, …} WebrtcProvider {…}

dmonad commented 3 years ago

It's possible it's a race condition since I'm just doing it locally between two browsers or it's something stranger with the devserver updating.

This is probably the reason. A devserver often duplicates created objects. Sorry, it is impossible for me to debug this issue. Unless you provide a step-by-step example for how I can reproduce this (without some react dev-server setup) I can't help you.

Can you maybe create a simple project (maybe fork https://github.com/yjs/yjs-demos) and describe the steps to reproduce this issue?

disarticulate commented 3 years ago

it appears to be chrome specific. I upgraded the demo here:

https://github.com/disarticulate/y-webrtc/tree/master/demo

Reproduce:

open 2 chrome threads (not in same window):

/usr/bin/chromium-browser --user-data-dir=$root/chrome1 --password-store=basic http://localhost:8080/demo/ &
/usr/bin/chromium-browser --user-data-dir=$root/chrome2 --password-store=basic http://localhost:8080/demo/ &

open a firefox browser (just to double check)
wait till added:webrtc:[peerId]
close chrome or refresh chrome

observe console:

removed:webrtc:[fb8ba1ec-759c-4b57-a6d5-fb5f9a19b780] index.js:13:12
synced! 
Object { synced: false }
index.js:25:10
Object { _readableState: {…}, readable: true, _events: {…}, _eventsCount: 2, _maxListeners: undefined, _writableState: {…}, writable: true, allowHalfOpen: false, _id: "f240014", channelName: "7a260e731074938719ec8719f2e19adf96eb1458", … }
SimplePeerExtended.js:44:12
added:webrtc:[fb8ba1ec-759c-4b57-a6d5-fb5f9a19b780]

You may want to use the standard SimplePeer if you think I did something untoward to it.

I'll comment on the rest of that code in the other issue I've got open https://github.com/yjs/y-webrtc/issues/20

yjs / y-webrtc

'peers' event on on SignalingConn emits 'added' peer from peer's close event #23