Skip to content
This repository has been archived by the owner on Jan 25, 2022. It is now read-only.

cleanupSome on main thread questions #197

Closed
syg opened this issue Apr 1, 2020 · 18 comments
Closed

cleanupSome on main thread questions #197

syg opened this issue Apr 1, 2020 · 18 comments

Comments

@syg
Copy link
Collaborator

syg commented Apr 1, 2020

@kmiller68 brought up the concern that FinalizationRegistry#cleanupSome is counter to the web API philosophy where we shouldn't encourage synchronous work. While the spec already does allow cleanupSome to always be a no-op, there is a practical desire for the web browsers to align here. In other words, if Safari always no-ops cleanupSome on the main thread but Chrome always processes finalizers, that runs a larger interop risk in practice than cleanupSome behaving differently due to GC timing anyway.

There are two ways forward:

  1. Remove cleanupSome entirely. I strongly, strongly consider this a non-starter. It is well-motivated for the web worker use case and non-web use cases, and the current TC39 consensus is that we agreed to its usefulness.
  2. Let the host decide in what contexts cleanupSome is available or is able to do something:
    1. Add a host hook that lets the host (HTML) decide if cleanupSome should throw, e.g. on the main thread.
    2. Make cleanupSome normative option for hosts at Realm-creation time, like what TC39 just decided for SharedArrayBuffer. This makes feature testing slightly easier.
  3. Spin off cleanupSome and the question of if and how to allow synchronous processing of finalization events to a different proposal and demote it to Stage 2.

Concretely, I propose 2.ii to be fully fleshed out in the HTML integration patch, since Keith's concern is mainly a host concern in HTML where the main thread has distinctly different API availability expectations than worker threads.

I now propose option 3, given the severity of concern from Apple. Apple believes allowing synchronous processing of finalization events to be counter to the goals of the web platform, and I think that question is orthogonal enough to be iterated upon without holding up the rest of the WeakRef proposal.

Thoughts from @codehag for Firefox also much appreciated.

@syg
Copy link
Collaborator Author

syg commented Apr 2, 2020

Also cc @erights

@kmiller68
Copy link

kmiller68 commented Apr 2, 2020

I guess I'd like to clarify my position a bit. It's not so much about synchronous work as it is about the current architecture of the Web as a whole. In particular, the Web Platform is largely event driven and previous systems that try deviate from this have resulted in poor user experiences. Namely, Plugins. Looking from the the web platform as a whole, we think that the use cases for this promote an architectural philosophy differs from the web as it exists today, which result in bad user experiences. While I respect that other platforms for JS exist (and thrive) I don't want that to come at the expense of the Web Platform.

In order for cleanupSome to proceed on the web, we'd like to see a cohesive transpilation proposal that covers: integration of the html event loop (including idle sleep), event handling, animation frames, message passing, tasks and micro tasks, accessibility, and termination. Some of these make sense for JS in isolation but this proposal also has implications for WebAssembly. Unfortunately, this means the WebAssembly issues will need to be addressed as well. So, we are going to ask that the committee consider separating cleanupSome from the rest of WeakRefs tomorrow.

I'd also like to apologize for the lateness of this feedback. These overarching concerns only came up once I started sharing this proposal with the rest of the WebKit team during implementation. In isolation, this proposal seemed innocuous (modulo compatibility).

@erights
Copy link
Contributor

erights commented Apr 2, 2020

It is up to the host to determine what gets cleaned up during a cleanupSome. Within this proposal, browsers can, and w3c could even specify, that on the main event loop cleanupSome cleans up nothing. This is within the behavior allowed to hosts by this proposal. cleanupSome in long lived workers would not have that restriction.

@erights
Copy link
Contributor

erights commented Apr 2, 2020

This isn't quite the same an any of @syg 's bullets. But I think it is similar enough that it is part of the same menu of choices. The difference is this one fits within the current proposal without changes or additions.

@kmiller68
Copy link

kmiller68 commented Apr 2, 2020

I think that practically making cleanupSome be a no-op is going to result in compatibility nightmares. This is because the naive application is going to implement their finalization registry as:

let globalTable = new Map();
new FinalizationRegistry((holdings) => {
    globalTable.remove(holdings);
}

So for any application that does not test in a browser that does a no-op cleanupSome() this finalizer will simply leak because the application doesn't return to the event loop.

@codehag
Copy link
Contributor

codehag commented Apr 2, 2020

To answer the initial question: cleanupSome was really intended for workers, so from our perspective, the proposal to throw on the main thread or to leave it up to the host are both fine and we would support it.

In order for cleanupSome to proceed on the web, we'd like to see a cohesive transpilation proposal that covers: integration of the html event loop (including idle sleep), event handling, animation frames, message passing, tasks and micro tasks, and termination. Some of these make sense for JS in isolation but this proposal also has implications for WebAssembly. Unfortunately, this means the WebAssembly issues will need to be addressed as well. So, we are going to ask that the committee consider separating cleanupSome from the rest of WeakRefs tomorrow.

The WebAssembly use case is important to us. We want to make sure that long running calculations that do not return to the event loop can be supported. Before we move to separating this into it's own proposal, I want to make sure that this will still be possible to support and I want to make sure that we can come to an agreement about that.

@mhofman
Copy link
Member

mhofman commented Apr 2, 2020

Looking from the the web platform as a whole, we think that the use cases for this promote an architectural philosophy differs from the web as it exists today, which result in bad user experiences.

I believe that ship has already sailed with SAB and Atomics. Realistically workers are able to have this kind of architecture today on the web.

Since we're talking about making SAB normative optional, can't we make cleanupSome normative optional, and have HTML spec that it is only available in CanBlock workers with SAB enabled?

@annevk
Copy link
Member

annevk commented Apr 2, 2020

Why not make it normative that cleanupSome throws (or no-ops) if [[CanBlock]] is false? That way you're not leaving it up to hosts, nothing is left up to interpretation, and HTML will automatically do the right thing (window / service worker agents have it set to false).

@erights
Copy link
Contributor

erights commented Apr 2, 2020

Why not make it normative (web normative, not js normative) that when [[CanBlock]] is false cleanupSome cleans up nothing? Then there isn't an irritating throw that code needs to tolerate.

@kmiller68
Copy link

The WebAssembly use case is important to us. We want to make sure that long running calculations that do not return to the event loop can be supported. Before we move to separating this into its own proposal, I want to make sure that this will still be possible to support and I want to make sure that we can come to an agreement about that.

The concern is that even in a Worker cleanupSome() is problematic. Because the main motivating use case for this is custom event loop style apps there is strong concern cleanupSome() promotes a class of web applications with a history of poor user experience. That's why I'd like to see arguments as to why this iteration will be different. For what it's worth, I understand and can relate with the developers desire to use such frameworks but I'm not sure that outweighs the costs to users.

@erights
Copy link
Contributor

erights commented Apr 2, 2020

@kmiller68 are you saying that even if this style is used only in a worker that it would create a poor user experience? How?

@kmiller68
Copy link

kmiller68 commented Apr 2, 2020

@kmiller68 are you saying that even if this style is used only in a worker that it would create a poor user experience? How?

Yes, for the same reasons that previous black-box custom event loop apps (e.g. flash) did. If the goal is to enable those types of apps, I'd like to know why we're not retreading the same road.

@mhofman
Copy link
Member

mhofman commented Apr 2, 2020

Those type of custom event loop programs are possible today without cleanupSome.

I'm actually not sure cleanupSome would be that useful to those custom event loops programs. WeakRef.deref() prevents collection during a synchronous execution, so those programs would have to use finalizers without WeakRef.

@erights
Copy link
Contributor

erights commented Apr 2, 2020

@mhofman thanks for pointing that out. I'd forgotten about that. Yes, that's why we made the separation (initially, in a more awkward way). The JS-to-wasm acyclic gc issue doesn't need weakrefs, just finalization. Needing weakrefs during a turn is indeed useful functionality explicitly not available to long-running single job programs.

@erights
Copy link
Contributor

erights commented Apr 2, 2020

The functionality of weakness is not available. the api still is with its normal in-turn meaning is still available of course.

@erights
Copy link
Contributor

erights commented Apr 2, 2020

In any case, at tc39 we settled this today as literally normative optional. The signal is only the presence or absence of the cleanupSome property. Now it is up to each host or host standard to decide for itself whether it is present or absent or conditionally present on that host. Closing.

@erights erights closed this as completed Apr 2, 2020
@annevk
Copy link
Member

annevk commented Apr 3, 2020

How is that good for cross-host libraries? Also, how does that address your point above about allowing it to no-op so you can call it either way?

@erights
Copy link
Contributor

erights commented Apr 3, 2020

Hi @annevk I prefer the no-op version. But the normative-optional feature testing version was the one we were able to jointly agree on. I am happy enough with it --- especially now that JS has optional chaining!

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

No branches or pull requests

6 participants