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

MediaConnection.close() does not invoke remote peer's close method #1089

Closed
samhirtarif opened this issue Jun 4, 2023 · 5 comments
Closed

Comments

@samhirtarif
Copy link

samhirtarif commented Jun 4, 2023

Issue

The problem is straight forward, when a local peer calls the close method on MediaConnection, the remote peer is not informed in any way, which results in the remote peer's close method not being invoked and the MediaConnection on the remote peer's end staying open indefinitely until they close it themselves.

Proposed resolution

The MDN docs have a guide on Signaling and video calling that has a section for "HANGING UP" a call. The way they have shown the implementation should be is that the user that hang's up should send a message to the remote peer through a server that they've hung up.

image
Screen shot from MDN docs

This gave me the idea of introducing a similar pattern to the peerjs implementation. Which is, introducing a HangUp message in the ServerMessageType enum, that is sent to the remote peer from the cleanup method in the Negotiator class. The remote peer would then invoke their own close method when they receive this message.

Introducing this message type would also require us to handle this message on the peerjs-server end by registering a handler in the handlersRegistry presumably.

TL;DR for Proposed resolution

Signal to the remote peer through a new ServerMessageType called HangUp when the local peer calls the close method, the remote peer would invoke their own close method on receiving this message. This would also require changes on peerjs-server to add a message handler for the new message type.


This is a solution that I have been working on and I just wanted to get opinions and recommendations from the peerjs core development team on this too! Thanks in advance! 😃

@samhirtarif
Copy link
Author

samhirtarif commented Jun 20, 2023

Update: I've tried out the proposed solution and it works.
@jonasgloning - any suggestions or recommendations? Or should I go ahead with creating pull requests for both repos?

@jonasgloning
Copy link
Member

Thanks for pinging me @samhirtarif, and sorry for getting to this so late.

Your solution will work, but I'm still a bit hesitant, as the current expectation might be that you can .disconnect() from the server and still have all functionality for existing connections.
Someone else proposed a separate DataChannel as a way to transfer the hang-up message.

I'll search if there is a way without a separate channel. I'll get back to you soon.

@samhirtarif
Copy link
Author

samhirtarif commented Jun 21, 2023

@jonasgloning - Thank your for taking the time to respond to this
When you say ".disconnect() from the server" is that disconnection from the peerjs socket server?
And I'll be waiting to hear back from you, will try to find other ways too to fix this

jonasgloning added a commit that referenced this issue Jun 22, 2023
github-actions bot pushed a commit that referenced this issue Aug 14, 2023
# [1.5.0-rc.1](v1.4.8-rc.1...v1.5.0-rc.1) (2023-08-14)

### Bug Fixes

* **datachannel:** sending order is now preserved correctly ([#1038](#1038)) ([0fb6179](0fb6179)), closes [#746](#746)
* **deps:** update dependency peerjs-js-binarypack to v1.0.2 ([7452e75](7452e75))
* **deps:** update dependency webrtc-adapter to v8.2.2 ([62402fc](62402fc))
* **deps:** update dependency webrtc-adapter to v8.2.3 ([963455e](963455e))
* **MediaConnection:** `close` event is fired on remote Peer ([0836356](0836356)), closes [#636](#636) [#1089](#1089) [#1032](#1032) [#832](#832) [#780](#780) [#653](#653)

### Features

* **DataConnection:** handle close messages and flush option ([6ca38d3](6ca38d3)), closes [#982](#982)
* **MediaChannel:** Add experimental `willCloseOnRemote` event to MediaConnection. ([ed84829](ed84829))
* MsgPack/Cbor serialization ([fcffbf2](fcffbf2))
@github-actions
Copy link

🎉 This issue has been resolved in version 1.5.0-rc.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

github-actions bot pushed a commit that referenced this issue Sep 3, 2023
# [1.5.0](v1.4.7...v1.5.0) (2023-09-03)

### Bug Fixes

* **datachannel:** sending order is now preserved correctly ([#1038](#1038)) ([0fb6179](0fb6179)), closes [#746](#746)
* **deps:** update dependency @swc/helpers to ^0.4.0 ([a7de8b7](a7de8b7))
* **deps:** update dependency cbor-x to v1.5.4 ([c1f04ec](c1f04ec))
* **deps:** update dependency eventemitter3 to v5 ([caf01c6](caf01c6))
* **deps:** update dependency peerjs-js-binarypack to v1.0.2 ([7452e75](7452e75))
* **deps:** update dependency webrtc-adapter to v8 ([431f00b](431f00b))
* **deps:** update dependency webrtc-adapter to v8.2.2 ([62402fc](62402fc))
* **deps:** update dependency webrtc-adapter to v8.2.3 ([963455e](963455e))
* **MediaConnection:** `close` event is fired on remote Peer ([0836356](0836356)), closes [#636](#636) [#1089](#1089) [#1032](#1032) [#832](#832) [#780](#780) [#653](#653)
* **npm audit:** Updates all dependencies that cause npm audit to issue a warning ([6ef5707](6ef5707))

### Features

* `.type` property on `Error`s emitted from connections ([#1126](#1126)) ([debe7a6](debe7a6))
* `PeerError` from connections ([ad3a0cb](ad3a0cb))
* **DataConnection:** handle close messages and flush option ([6ca38d3](6ca38d3)), closes [#982](#982)
* **MediaChannel:** Add experimental `willCloseOnRemote` event to MediaConnection. ([ed84829](ed84829))
* MsgPack/Cbor serialization ([fcffbf2](fcffbf2))
* MsgPack/Cbor serialization ([#1120](#1120)) ([4367256](4367256)), closes [#611](#611)
@jonasgloning
Copy link
Member

Hey @samhirtarif, this should be fixed in version v1.5 (just published) via an auxiliary data channel.
This way you can disconnect from the peerjs socket server and still retain all functionality for existing connections.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants