0.19.0 regression: onPeerLeave not called when a peer refreshes

[jeremyckahn]

Thanks for all the hard work on the 0.19.0 release!

I'm working to upgrade Chitchatter to use the latest Trystero, and it's mostly working great. However I'm noticing that onPeerLeave is no longer called when a peer refreshes or otherwise closes the page. This appears to be a regression from 0.18.0.

You can see the correct behavior with 0.18.0 at the Production URL: https://chitchatter.im/

And the new, broken behavior at this preview URL: https://chitchatter-git-feature-t-0f1de0-jeremy-kahns-projects-98e6f140.vercel.app/

To reproduce the issue:

• Join the same room from separate browsers
• Refresh one browser
• Observe that once the refreshed peer rejoins the room, they appear twice in the room for the peer that didn't refresh.

You can see who is in the room by opening the right-hand menu:

I've done some debugging and couldn't see onPeerLeave getting called anywhere when a peer leaves their page. Any idea what might be causing this?

[dmotz]

Thanks for flagging. I noticed it works as expected in Chrome but not Firefox. I'm not sure what change would've introduced this regression but it's probably at the peer disconnection event level. I'll look into a fix.

One idea for your app (even once the fix lands) is to implement a heartbeat between peers to detect zombie connections. You could do something like a periodic Promise.race() between room.ping(peerId) and a setTimeout promise that resolves after a certain number of seconds where a connection might be considered stalled. If the timeout beats the ping response, you could remove the peer from the list. (You don't have to use ping(); you could make your own call and response action). I'm also considering implementing this behavior at the library level and curious what you think.

[jeremyckahn]

Thanks for looking into a fix for this @dmotz!

As for zombie connection handling, I had figured that this was handled one way or another in pre-0.19.0 versions. Or at least, zombie connections would eventually disappear as expected. Personally, I was pretty satisfied with how it was working before. A quicker zombie detection would be certainly be nice, but it's questionable to me if it would be worth the additional bandwidth and processing overhead. That said, I think your proposed solution would work quite well and don't have alternative approach offhand. It seems worth experimenting with!

From my perspective as a user of Trystero, it would be ideal if your strategy was implemented at the library level. If nothing else it would save me (and others) from having to re-implement general zombie detection logic in every downstream project. It also strikes me as a Trystero concern (rather than application concern) because it has to do with peer networking implementation details.

FWIW, I don't have any expectation of improved zombie connection handling in Trystero. I'd be satisfied with just seeing this regression be addressed. I'd certainly welcome any performance improvements you're interested in making, and I'm happy to help test things out if you'd like that!

[dmotz]

This should be fixed in the latest release (0.20.0), but let me know if you still see issues.

[jeremyckahn]

This is working great. Thanks for the fix @dmotz! I'm excited to be on the latest version of Trystero. :)