w3c / webrtc-pc

WebRTC 1.0 API
https://w3c.github.io/webrtc-pc/
Other
436 stars 115 forks source link

Promises on the operations chain may never settle #2973

Open chrisguttandin opened 4 months ago

chrisguttandin commented 4 months ago

Please consider the following example:

const peerConnection = new RTCPeerConnection();

peerConnection.createDataChannel('label');
peerConnection
    .setLocalDescription()
    .then(() => console.log('resolved'), () => console.log('reject'));
peerConnection.close();

It will log nothing in all browsers. The promise just never settles. It seems to be inline with the spec.

https://w3c.github.io/webrtc-pc/#chain-an-asynchronous-operation

In 7.1. it says "If connection.[[IsClosed]] is true, abort these steps." Maybe it should reject the promise with an InvalidStateError instead as it would do when the connection is already closed by the time the promise gets added to the operations chain.

The following snippet will invoke the error callback as expected.

const peerConnection = new RTCPeerConnection();

peerConnection.createDataChannel('label');
peerConnection.close();
peerConnection
    .setLocalDescription()
    .then(() => console.log('resolved'), () => console.log('reject'));

The examples are of course very contrived but I can imagine that this behavior is responsible for a lot of difficult to debug memory leaks.

jan-ivar commented 4 months ago

The examples are of course very contrived but I can imagine that this behavior is responsible for a lot of difficult to debug memory leaks.

What memory leaks? AFAIK there's no rule that promises must settle. They're just callbacks never called. Easily GC'ed.

chrisguttandin commented 4 months ago

Sorry, for the confusion. I meant memory leaks caused by the unexpected behavior not by the promise itself. The following snippet will for example trigger out of memory errors quite quickly.

const arrayBuffers = [];

setInterval(() => {
  const peerConnection = new RTCPeerConnection();

  arrayBuffers.push(new ArrayBuffer(100_000_000));

  peerConnection.createDataChannel('label');
  peerConnection
      .setLocalDescription()
      .finally(() => arrayBuffers.pop());
  peerConnection.close();
}, 100);

But I acknowledge that this snippet could be considered buggy since it relies on the promise to settle. Maybe it's just me but I never thought that this edge case needs to be handled.

jan-ivar commented 4 months ago

Can you clarify what edge case needs to be handled, and provide support for the behavior being "unexpected"?

It's possible we made the wrong decision years ago, but this has shipped in all browsers, so changing behavior now would come at a web compat cost of triggering unexpected errors on close where none are triggered today. So I think we'd need a pragmatic reason to change it at this point.

If this is causing a lot of difficult to debug memory leaks, that would be concerning and might be a reason to reconsider this design, but I'm not sure the example given is sufficient to conclude this is happening.

In practice, applications are encouraged to use peerConnection.onnegotiationneeded to isolate its negotiation code, and the application knows when it calls close() which the browser never calls except on document unload.