DataConnection.on("close") listeners are not fired on device when closing the dataConnection

[Pitouli]

Please, check for existing issues to avoid duplicates.

• [x] No similar issues found.

What happened?

When I manually close the dataConnection by doing conn.close(), the listener that I have previously attached is not called ; it is called only when it is the remote peer that closes the connexion.

Note that it is especially visible in scenarios were the connexion has never been successfully openned (for example, you tried to connect to an unavailable peer, and then you close this failed connexion).

Note also that the connexion is correctly removed from peer.connexions after conn.close()

How can we reproduce the issue?

1. Initiate a Peer
2. Try to connect to an unavailable peer (connect to a random ID)
3. You will see that the new connexion effectively appears in peer.connexions
4. Add listeners on the connexion (in particular on "close" and "error")
5. You will see that a "peer-unavailable" error is raised, but on the peer level and not the conn level
6. Later, call conn.close()
7. Check peer.connexion, the conn has been removed. But your listener has not been called

What do you expected to happen?

On step 5, I would like the error to be raised on the conn itself, and not the peer (but I think this issue is already discussed here: https://github.com/peers/peerjs/issues/1281 )

On step 7, I would like the listeners to be called.

Note that in the doc, it is said about the "close" event of the DataConnection:

Emitted when either you or the remote peer closes the data connection.

Environment setup

• OS: Windows 10
• Platform: React 18
• Browser: Chrome

Is this a regression?

I do not know

Anything else?

I have made a workaround:

const connOnClose = (extendedClose = false) => {
  // The code I want to execute onClose
};
conn.on("close", connOnClose);
conn.extendedClose = () => {
  conn.close();
  connOnClose(true);
};

It does the trick, but I feel like it should work with the close() function directly

[jonasgloning]

Thank you for the detailed report!

The close listener is called when you manually close() the connection — but only if the connection transitions from the open state to the closed state.

That's why it works when closed from the remote peer, but not when the connection never established: If the connection never opened, it cannot close, because it was never in an open state!

I think there's a mismatch in expectations here: Should .close() emit events even for connections that never opened? One could make the case for either behaviour. However, I wouldn't classify this as a bug.

Do you have a suggestion on how to either document this more clearly, or change the behaviour? We could also add something like this.close({ emitCloseEvent: "always" }). I'm really open for suggestions here.

[Pitouli]

Hello!

Thank you for your answer.

If we keep the current behaviour, I think we could rewrite the doc like this:

Emitted when either you or the remote peer closes the data connection if opened

About the necessity to make an evolution or not, I believe it is linked to a better error management.

The issue I face currently is that when I attempt to open the connection, if there is an error, it is triggered on the peer itself and not on the errored connection.

So the connection stays in my list of "opening connections", and I have a mechanism that consider after 10s that the connection is "probably failed" and close it. And that's why I would have liked the close event to be triggered so the cleaning mechanism apply.

But if the error was triggered on the data connection itself, I could reuse on the "error event" the same cleaning mechanism.

But we would still have one uncovered case: if you attempt to open a connection, and close it BEFORE it had time to be opened or in error, neither close event nor error event would be triggered.

So perhaps the idea would be to create an additional "destroyed" or "removed" event that is triggered when the connection is removed from the list of the peer.connections, either after a closing or an error.

What do you think?