Handle FinalizationRegistry

[jridgewell]

This snapshots the ambient AC state when calling fr.register(), to be restored when invoking the FR's callback on that registered cell.

---

I also removed our changes of HostMakeJobCallback. First, because it felt awkward to refer to the JobCallback's [[AsyncContextSnapshot]] inside NewPromiseReactionJob and NewPromiseResolveThenableJob. And second, because it actually causes a memory leak with FinalizationRegistry.

The FR calls HostMakeJobCallback (causing it to snapshot) during its constructor to prepare the callback to be used for jobs later on. But that snapshot will never be used, we'll only use the registration time snapshot. So I had to move the snapshotting closer into PerformPromiseThen.

[andreubotella]

I thought this PR was meant to fix the regression introduced in #41. However, before #41, FinalizationRegistry callbacks were called with the FR construction context, rather than with the context at the time the register method was called. Have we actually discussed that the behavior in this PR is the right one?

This is actually similar to some events in the web:

const asyncVar = new AsyncContext.Variable();

const image = document.getElementById("image");  // <img id="image">

asyncVar.run("foo", () => {
  image.addEventListener("load", () => {
    console.log(asyncVar.get());
  });
});

asyncVar.run("bar", () => {
  image.src = "./some/image.jpg";
});

Here we have a registration-time context (the registration of the load event, or the FR construction with a callback), and a separate context (the call to register, or the setting of the image's src) which asynchronously triggers the call to the registered callback.

It's not clear to me which of the two is the right behavior, and it clearly ties into the behavior of events in the HTML integration.

[mhofman]

I remember this being discussed in Matrix. I believe register time is the correct one. Furthermore it's easy to do FR construction time by simply wrapping the callback.

Edit: the other direction is more tricky, requiring to create a snapshot on registration, pass it in the held value, and run from the snapshot in the callback.

[andreubotella]

I remember this being discussed in Matrix. I believe register time is the correct one. Furthermore it's easy to do FR construction time by simply wrapping the callback.

I think I misunderstood you when this was discussed, so I didn't realize there was a difference wrt the previous behavior.

By the way, the terminology gets confusing here because for events, "registration-time" is the time at which you register the event by e.g. calling addEventListener, which would be equivalent to FR construction. Calling register would be an asynchronous kind of "call-time".

Edit: the other direction is more tricky, requiring to create a snapshot on registration, pass it in the held value, and run from the snapshot in the callback.

Spec-wise it's not more tricky, since the snapshot doesn't need to be held in the FR's [[Cells]]. In the current spec text, HostMakeJobCallback takes the snapshot and stores it alongside the callback, so it could be swapped in CleanupFinalizationRegistry() in the same way it is swapped in NewPromiseReactionJob.

[mhofman]

By the way, the terminology gets confusing here because for events, "registration-time" is the time at which you register the event by e.g. calling addEventListener, which would be equivalent to FR construction. Calling register would be an asynchronous kind of "call-time".

Ah yes I meant FR register. It's not the same as addEventListener, but it's not equivalent an async dispatch either. IMO semantically it's more like if the program did addEventListener for target, but with the event handler shared and added earlier.

Spec-wise it's not more tricky,

I was strictly taking the POV of the API consumer. It's harder to get the register time semantics manually.

the snapshot doesn't need to be held in the FR's [[Cells]]

I'm confused, if we want FR register time, doesn't the spec need to store the Snapshot in each cell?
I probably should read the PR

[andreubotella]

the snapshot doesn't need to be held in the FR's [[Cells]]

I'm confused, if we want FR register time, doesn't the spec need to store the Snapshot in each cell? I probably should read the PR

That's right, it would need to be stored in each cell. But I thought by "the other direction" you meant construction time.

[littledan]

I'd rather use the construction-time AsyncContext snapshot here, just for simplicity. Are there any cases that we're aware of where it's important to have the semantics described in this PR?

[jridgewell]

I think using construction-time is the incorrect choice, because observers like this (and DOM observers like MutationObserver) should be held in the module scope. If it's in the module scope, the ambient context is just empty, and that's not useful.

I think choosing the context during the call to register is directly analogous to choosing the context when you attach an event listener. Andreu had brought up during last meeting that "registration-time" to him was when you passed the callback, but I think that's incorrect. Registration is when I invoke the method to be notified of a future event, eg fr.register() or el.addEventListener().

[littledan]

Not passing the callback at the time you register is a big difference from addEventListener. I think the consistency arguments can be made in both directions. Is there any actual utility to the semantics of this patch? If not, I really prefer the option which is simpler from an implementation and specification perspective. Note that it is always possible to build a wrapper around FinalizationRegistry which applies the semantics in this patch.

If we can’t come to a conclusion on this thread, let’s discuss this at the next AsyncContext meeting.

[Jamesernator]

because observers like this (and DOM observers like MutationObserver) should be held in the module scope

Other observers batch things anyway (which FinalizationRegistry was originally proposed to do as well, but was dropped at some point) so having a "registration time" for DOM observers is just a non-possibility anyway.

[jridgewell]

Other observers batch things anyway (which FinalizationRegistry was originally proposed to do as well, but was dropped at some point) so having a "registration time" for DOM observers is just a non-possibility anyway.

That's interesting. I've always assumed it only batched mutations per-observe calls (so two appendChilds on a subtree observe would only fire once per tick). I didn't realize all observe calls are invoked at once (so subtree observe and a separate attributes observe would also only fire once).

So it really isn't possible to automatically maintain "registration" time per observe call, and users that need it would have to use a separate WeakMap or something to handle Snapshots. Given that, I'm ok with either approach.

[mhofman]

and users that need it would have to use a separate WeakMap

You don't even need to go that far.

class FinalizationRegistryWithSnapshot extends FinalizationRegistry {
  constructor(callback) {
    super(({snapshot, value}) => {
      snapshot.run(callback, value);
    })
  }
  register(target, value, token) {
    return super.register(target, {value, snapshot: new AsyncContext.Snapshot()}, token);
  }
}

[littledan]

In the AsyncContext call today, the group apparently decided on construction-time snapshot semantics. (Please correct me if I'm mistaken.)

[jridgewell]

You don't even need to go that far.

I was referencing MuationObserver in that comment, which would require a WeakMap to hold a #[element, registration-type] -> Snapshot mapping.

[jridgewell]

Updated to use creation-time context.

[andreubotella]

This needs an <ins>.