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

Would it be helpful to restrict use of AudioWorkletProcessor.port().postMessage() in order to facilitate garbage collection? #2060

Closed
karlt opened this issue Sep 15, 2019 · 9 comments · Fixed by #2116
Assignees
Projects
Milestone

Comments

@karlt
Copy link
Contributor

karlt commented Sep 15, 2019

Consider the VU Meter Node example, but followed by oscillator.stop() and cancelAnimationFrame(drawMeter). The main global scope has no references to vuMeterNode or to vuMeterNode.port, the input to the associated processor falls silent, and the process() method returns false. process() will not be called again, and the AudioWorkletGlobalScope has no references to the processor or its port.

Theoretically, an implementation could determine that vuMeterNode, its processor, and their MessagePorts will not be accessed again, and so could recover all resources associated with vuMeterNode. Currently, however, no browsers have the capability to do this.

If an entangled port portA can be accessed from script and its [[RemotePort]] portB has a listener, then portA must keep portB alive to receive messages. If portB may receive messages, then the message listener is able to send messages back. If portA has a listener, then portB must keep portA alive to receive messages. There is an ownership cycle.

To my knowledge, no browser currently has the ability to detect when such cycles are garbage if portA and portB are on different threads, as would be the case with AudioWorkletNode. Detection of otherwise unreferenced cycles of this nature may require significant architectural changes, one complication being that an approach that required stopping both threads to analyse ownership graphs would not be acceptable when a realtime audio thread is involved.

One alternative option might be to non-normatively document that a client which listens on AudioWorkletNode.port should call close() on either of the ports to allow resources to be recovered before page unload. I feel like this would be passing the buck to the client to find a solution. I'd like to consider whether there is a better option. (The second note at https://html.spec.whatwg.org/multipage/web-messaging.html#ports-and-garbage-collection is similar to this option but misleads that lack of a close() call can lead to only transient memory usage.)

The proposal to consider is whether AudioWorkletProcessor.port().postMessage() would throw when invoked if neither during the processFunction invocation for the associated AudioWorkletNode in the rendering loop nor during message or messageerror event handlers for this AudioWorkletProcess.port.
When an AudioWorkletNode is not actively processing, and there are no messages in flight for its ports, this restriction would allow an implementation to more easily determine that the AudioWorkletProcessor.port could not send messages and so would not need to keep its entangled AudioWorkletNode.port alive. When all ownership is in the direction from main global scope to AudioWorkletGlobalScope, there are no inter-thread cycles, main global scope objects can be detected as garbage without consideration of associated AudioWorkletGlobalScope objects, AudioWorkletNodes can be released in the same way as other AudioNodes, and their associated AudioWorkletGlobalScope objects can then also be released.

What are people's thoughts?

An implementation cannot assume that AudioWorkletProcessor.port.postMessage() would only be called during a callback on this processor or its port because a client can share a reference to the port via the shared AudioWorkletGlobalScope.

@svgeesus svgeesus added this to the Web Audio V1 milestone Sep 16, 2019
@rtoy rtoy added this to Untriaged in V1 via automation Sep 16, 2019
@hoch hoch moved this from Untriaged to In WG Discussion in V1 Sep 16, 2019
@padenot
Copy link
Member

padenot commented Sep 16, 2019

F2F summary, this is a serious issue, and no real solution has been found.

Because another MessagePort can be sent inside the scope, even if the problem outlined in the initial comment were to be fixed, a leak would be possible still.

The group will try to get external advice on how to go forward here, and needs more time to decide what to do.

@padenot
Copy link
Member

padenot commented Oct 31, 2019

AudioWG call: @rtoy to find someone that could help the group.

@developit
Copy link

This is just my 2¢, and it's likely I'm missing some context, but it feels like a model where the main thread is considered the port owner would be in keeping with the worklet lifecycle model. Would it be possible to spec/implement queueing for any postMessage() invocations outside the bounds of processFunction or the other processor entrypoints?

@padenot
Copy link
Member

padenot commented Nov 5, 2019

How would one make the main thread the "port owner" ? The problem here is that the MessagePort has two ends, that can communicate, and thus keep closures, etc., alive.

Or maybe I don't understand?

@padenot
Copy link
Member

padenot commented Nov 14, 2019

AudioWG call:

How about, as a mitigation (not a solution), closing all MessagePort when close() is called on a BaseAudioContext?

@karlt
Copy link
Contributor Author

karlt commented Dec 2, 2019

That would be simpler for the author than closing all MessagePorts on at least one side, and I don't think there's any reason we'd need to support messages after close().
This could be a solution for scenarios where AudioWorkletNodes are intended to live for the duration of a realtime AudioContext.

So, I think that is reasonable but I hesitate because I don't know whether it is better to solve this for the portion of cases where it is easy to solve, or to leave it so that the author needs to be consistent about close()ing.

I also don't know whether it is better to try to mitigate this or to leave it to implementations to improve their GC algorithms, which is not likely to happen soon. I don't know how practical suitable algorithms would be.

@karlt
Copy link
Contributor Author

karlt commented Dec 3, 2019

Something to consider is that Chrome and Firefox both transition their OfflineAudioContexts to "closed" on completion of rendering. (I can't find this documented in the spec.)

It would be unhelpful to close MessagePorts on completion of offline rendering, because that would be the point where the client may wish to query state from the AudioWorkletNode.

Having different behavior for offline and realtime contexts in the "closed" state would be doable, but seem unexpected.

@rtoy
Copy link
Member

rtoy commented Dec 12, 2019

For an offline context, people could call suspend just before the end to do any final stuff. The last few frames may be missed, but that might be ok.

@padenot
Copy link
Member

padenot commented Dec 12, 2019

AudioWG Call resolution:
Add a note that explains the problem, and tell authors to close at least one end of the port.

padenot added a commit to padenot/web-audio-api that referenced this issue Dec 19, 2019
padenot added a commit to padenot/web-audio-api that referenced this issue Dec 19, 2019
padenot added a commit to padenot/web-audio-api that referenced this issue Dec 19, 2019
@padenot padenot assigned padenot and unassigned rtoy Dec 19, 2019
padenot added a commit to padenot/web-audio-api that referenced this issue Jan 7, 2020
V1 automation moved this from In WG Discussion to Done Jan 7, 2020
padenot added a commit that referenced this issue Jan 7, 2020
* Add a note about MessagePort listener and garbage collection

This fixes #2060.

* Fix typo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
V1
  
Done
Development

Successfully merging a pull request may close this issue.

5 participants