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

EventTarget.captureFallbackContext API #107

Open
nicolo-ribaudo opened this issue Dec 17, 2024 · 6 comments
Open

EventTarget.captureFallbackContext API #107

nicolo-ribaudo opened this issue Dec 17, 2024 · 6 comments

Comments

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Dec 17, 2024

Extracted from AsyncContext & events: alternative proposal

Ref #100


The proposed web integration runs event listener in the "dispatch context", and use the "empty context" as a fallback when there is no dispatch context available.

This clashes with one of the goals: having "isolated" parts of the applications, and being able to trace from which part of the application a given error came from. The trivial approach is to store the "island ID" in an AsyncContext variable, and reading it when errors happen.

The solution that we have been discussing is to provide an EventTarget.captureFallbackContext API that lets people say "take a snapshot now, and use that as the fallback context for all the event listeners registered inside it". For example:

const currentTeam = new AsyncContext.Variable();
addEventListener("unhandledrejection", () => {
  console.log("Rejection from team " + currentTeam.get());
});
currentTeam.run("one", () =>
  EventTarget.captureFallbackContext(teamOne)
);
currentTeam.run("two", () =>
  EventTarget.captureFallbackContext(teamTwo)
);

function teamOne() {
  button.addEventListener("click", () => {
    // this listener will never run in the root context:
    // if possible it will propagate the contex that
    // dispatched it (e.g. by a .click() call), but if
    // not it will fallback to the context that was active
    // when EventTarget.captureFallbackContext was called
    
    Promise.reject();
  });
}

function teamTwo() {
  // ...
}

This imposes less memory constraints than the approach of having event listeners always capture the registration-time context, because the expectation is that you'd define "fallbacks" much less frequently than how often you .run AsyncContext.Variables, and thus event listeners will end up capturing all the same (or a few) snapshot.

This "fallback context" is not a safe way to completely hide the root empty context: for example, a simple dynamic import is enough to get access to it. It's meant to reduce the cases where you don't want to see the root context, which most commonly happens in EventTarget and its subclasses.


There are a couple of open questions:

Explicitly pass the snapshot?

Instead of EventTarget.captureFallbackContext(fn), we might want to have EventTarget.withFallbackContext(snapshot, fn). This would make it more obvious that there is a snapshot being taken.

Support a way to get the fallback context?

#100 (comment) by @Jamesernator made me think: what if I want to just add one variable to the "fallback", rather than capturing whatever I have?

If there was a way to query the current fallback context (e.g. EventTarget.runFallbackContext()), then you'd be able to get the current context, update a variable, and set the new snapshot as the new fallback context:

const updatedFallback = EventTarget.runFallbackContext(() =>
  myVar.run("hello", () => new AsyncContext.Snapshot)
);

EventTarget.withFallbackContext(updatedFallback, ()=> {
  document.addEventListener("click", () => { ... });
});

Or maybe we could have more an API like EventTarget.updateFallbackContext(myVar, value, calback), although I'd prefer to only have the "primitive" that this can be built on top. Ideally, this should only happen if/after we get a way to update a Snapshot without running it.

andreubotella added a commit to andreubotella/proposal-async-context that referenced this issue Jan 7, 2025
legendecas pushed a commit that referenced this issue Jan 13, 2025
* Add web integration document

* Add link to task attribution issue

* Update web integration

* Update WEB-INTEGRATION.md

Co-authored-by: Nicolò Ribaudo <[email protected]>

* Apply suggestions from code review

Co-authored-by: Nicolò Ribaudo <[email protected]>

* Link to #107

* Add markdown link

---------

Co-authored-by: Nicolò Ribaudo <[email protected]>
@dead-claudia
Copy link

dead-claudia commented Jan 29, 2025

This imposes less memory constraints than the approach of having event listeners always capture the registration-time context, because the expectation is that you'd define "fallbacks" much less frequently than how often you .run AsyncContext.Variables, and thus event listeners will end up capturing all the same (or a few) snapshot.

You can structure snapshots such that they're cheap to capture and cheap to enter. You just have to move the iteration work to asyncVar.get(). I don't see the issue in encouraging that, especially since the iteration itself can often be avoided.

Here's a (currently untested) implementation of how one might do it:

interface ContextNode {
    parent: undefined | ContextNode
    key: object
    current: any
}

let snapshot: undefined | ContextNode
let head: undefined | ContextNode

const Hole: unique symbol = Symbol()
type Hole = typeof Hole

export class Variable<T> {
    #initial: T
    // both of these should be weakly held
    #snapshot: Hole | undefined | ContextNode = Hole
    #current: any

    constructor(initial: T) {
        this.#initial = initial
    }

    get(): T {
        // Likely branch in hot code, only fails on snapshot change
        // This also is taken when a variable is only used in the same snapshot's scope and that snapshot is just saved and restored
        if (snapshot === this.#snapshot) return this.#current

        this.#snapshot = snapshot

        let value = this.#initial
        let node = snapshot

        while (node) {
            if (node.key === this) {
                value = node.current
                break
            }
            node = node.parent
        }

        this.#current = value
        return value
    }

    with<R>(value: T, f: () => R): R {
        const prevSnapshot = this.#snapshot
        const prevValue = this.#current
        const prevHead = head
        head = {
            parent: prevHead,
            key: this,
            current: value,
        }
        // Ensures a guaranteed cache hit in `.get()`
        this.#current = value
        this.#snapshot = snapshot
        try {
            return f()
        } finally {
            this.#current = prevValue
            this.#snapshot = prevSnapshot
            head = prevHead
        }
    }
}

export class Snapshot {
    #state: undefined | ContextNode = head

    run<R>(f: () => R) {
        const prevSnapshot = snapshot
        const prevHead = head
        head = this.#state
        snapshot = this.#state
        try {
            return f(...args)
        } finally {
            head = prevHead
            snapshot = prevSnapshot
        }
    }
}

I'm obviously excluding the Async part of the implementation here, but async is just an extension of this.


Edit: worth mentioning that high-performance frameworks have a need for fast snapshot save/restore as well. It's not just the DOM who would stand to benefit.

@andreubotella
Copy link
Member

andreubotella commented Feb 14, 2025

We were internally discussing an iteration of this proposal, where rather than storing a whole fallback context, with values for all variables, an alternative approach is to only keep the current values for one or more variables that are explicitly passed to this API.

This makes more sense, since we expect that most users of this API would only care about their variables, and other libraries with their own variables might not expect the fallback. It also makes different libraries having their own fallback context with their own variables not overwrite each other's work.

Finally, this iteration is also better given some concerns we've heard from browsers about AsyncContext potentially creating memory leaks. If the fallback context is only created explicitly for a few variables, it would be expected that their values would be leaked (or rather, kept alive as long as there remain event listeners that were created in the fallback context); whereas leaking the values for unrelated variables from a different library wouldn't be expected.

const asyncVar1 = new AsyncContext.Variable();
const asyncVar2 = new AsyncContext.Variable();

asyncVar1.set("foo", () => {
  asyncVar2.set("bar", () => {

    EventTarget.captureFallbackContext([asyncVar2], () => {
      button.addEventListener("click", () => {

        console.log(asyncVar1.get());  // undefined
        console.log(asyncVar2.get());  // "bar"

      });
    });

  });
});

@nicolo-ribaudo
Copy link
Member Author

nicolo-ribaudo commented Feb 14, 2025

An implementation for the above could be similar to the following:

class EventTarget {
  static #fallbackValues = new AsyncContext.Variable({ defaultValue: new Map() });

  #listeners = new Map();

  captureFallbackContext(variable, cb) {
    const fallbackValues = new Map(EventTarget.#fallbackValues.get());
    fallbackValues.set(variable, variable.get());
    EventTarget.#fallbackValues.run(fallbackValues, cb);
  }

  addEventListener(name, cb) {
    if (!this.#listeners.has(name)) this.#listeners.set(name, []);
    this.#listeners.get(name).push({ cb, fallbackValues: EventTarget.#fallbackValues.get() });
  }

  __browserInternal__dispatchEventWithEmptyContext(event) {
    for (const { cb, fallbackValues } of this.#listeners.get(event.name) ?? []) {
      let fallbackValuesIterator = fallbackValues.entries();
      (function next() {
        const { done, value } = fallbackValuesIterator.next();
        done ? cb(event) : value[0].run(value[1], next);
      })();
    }
  }

  dispatchEvent(event) {
    for (const { cb } of this.#listeners.get(event.name) ?? []) {
      cb(event);
    }
  }
}

You can structure snapshots such that they're cheap to capture and cheap to enter. You just have to move the iteration work to asyncVar.get(). I don't see the issue in encouraging that, especially since the iteration itself can often be avoided.

It doesn't matter how efficient it is to do the cloning. Even if it's O(1), it's still going to capture the whole context and its memory cannot be reclaimed for as long as there is any event listener registered in it.

@dead-claudia
Copy link

@andreubotella By that point, wouldn't it be better to just have people pass that array via event listener option?

Also, I see two composability hazards, one of which would constrain the shape of this proposal's ultimate API:

  1. While yes, some libraries wouldn't expect a fallback, others may very much want the fallback. This could be achieved with some sort of registry or boolean flag. Also, if a library can't provide defined fallback behavior (even if it's throwing) and they fail to document this requirement, that's an issue with the library, and language features should not bend to that if they can help it.
  2. What about when users want to save and restore a context across a callback, but don't have a direct reference to the variable? In that case, they'd have to fall back to snapshot.wrap(listener), which renders the whole thing pointless. And it's worth noting that if async variables go the route of using syntactic bindings rather than exposed objects, you won't have this option regardless.

It doesn't matter how efficient it is to do the cloning. Even if it's O(1), it's still going to capture the whole context and its memory cannot be reclaimed for as long as there is any event listener registered in it.

@nicolo-ribaudo Good point on memory, and there is a leak of old values in my suggested implementation as well that would be awkward to mitigate.

Maybe a better way to handle it is to provide a withContext: true flag in the event listener options to have users opt into maintaining a snapshot, as sugar for new AsyncSnapshot().wrap(listener). That way, they're only paying the memory if they want to.

Web frameworks are likely to just always set that boolean on managed listeners, while users generally won't bother unless they want/need to. Users are in the best position to make that decision anyways, not library or framework developers. (And I say this as a long time framework developer who's contributed to a lot of libraries.)

Also, by using such a flag, it'll offer a way to enable support after this feature ships, making HTML integration at least temporarily divisible.

@andreubotella
Copy link
Member

By that point, wouldn't it be better to just have people pass that array via event listener option?

The point of this API was never to be used to wrap a single event registration. Instead, it was to wrap an async algorithm that is somewhat isolated from the rest of the code, so it can be distinguished from other async algorithms running concurrently. That algorithm might be complicated and it might contain multiple addEventListeners far away from the .run() that sets up the instance of the algorithm, potentially in third-party code that they don't control.

My go-to example for how this API would be useful is something like a server-side or edge runtime's internal code, which wants to keep track of request IDs so they're logged with console.log() calls. If the fallback context for this one variable with the request ID is captured in the runtime's internal code, then user code could register events, and the request ID would be properly shown:

// Runtime internal code
import { handler } from "./user-code.js";

const reqIdVar = new AsyncContext.Variable();
let nextId = 1;

function handleRequest(req) {
  reqIdVar.run(nextId++, () => {
    EventTarget.captureFallbackContext([reqIdVar], () => {
      handler(req);  // Calls user code
    });
  });
}

globalThis.console.log = (...args) => {
  logsWriter.write(new Date(), reqIdVar.get(), args);
};

Here the user code is unaware that the runtime is using AsyncContext, and if they have any event listeners, the request ID shouldn't be lost inside them.

  1. While yes, some libraries wouldn't expect a fallback, others may very much want the fallback. This could be achieved with some sort of registry or boolean flag. Also, if a library can't provide defined fallback behavior (even if it's throwing) and they fail to document this requirement, that's an issue with the library, and language features should not bend to that if they can help it.

If a library wants a fallback, they'd want it for a given variable (or possibly multiple), that either they construct themselves or which gets passed by the caller. In either case, they could always create a fallback context for that variable inside their code, right?

  1. What about when users want to save and restore a context across a callback, but don't have a direct reference to the variable? In that case, they'd have to fall back to snapshot.wrap(listener), which renders the whole thing pointless. And it's worth noting that if async variables go the route of Suggestion: use syntax for async variables #113, you won't have this option regardless.

How is "the whole thing" rendered pointless? snapshot.wrap(listener) was always allowed, and it's what libraries should be using if they need to save and restore whole contexts.

@dead-claudia
Copy link

That algorithm might be complicated and it might contain multiple addEventListeners far away from the .run() that sets up the instance of the algorithm, potentially in third-party code that they don't control.

@andreubotella I missed that, and had I caught it, my second part would've been way different.

I also see general use outside the Web. So what about this?

  • Let the AsyncSnapshot constructor accept an optional list of specific variables to preserve. That isolates the functionality so anyone can easily use it.
  • Have your EventTarget.captureFallbackContext API use (nullable) snapshots instead of variable lists (this simplifies implementation as well)

This gives users and frameworks the freedom to do any of the 3 possibilities (drop context, retain some context, retain all context) and make appropriate tradeoffs. It also plays better with syntax-based variables like I suggested in #113.

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

3 participants