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

Promises on the operations chain may never settle #2973

Open
chrisguttandin opened this issue May 16, 2024 · 3 comments
Open

Promises on the operations chain may never settle #2973

chrisguttandin opened this issue May 16, 2024 · 3 comments

Comments

@chrisguttandin
Copy link

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
Copy link
Member

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
Copy link
Author

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
Copy link
Member

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.

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

No branches or pull requests

2 participants