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

Does this introduce a function coloring problem? #102

Open
rictic opened this issue Jul 29, 2024 · 11 comments
Open

Does this introduce a function coloring problem? #102

rictic opened this issue Jul 29, 2024 · 11 comments

Comments

@rictic
Copy link

rictic commented Jul 29, 2024

This proposal introduces a new question whenever calling a function: what context should this function be run in?

There isn't a principled way of making this decision.

Consider a class:

import {f, g} from './qux.js';

class Foo {
  constructor() {
    // ...
  }

  bar() {
    f();
    g();
  }
}

When we call f and g in bar(), this class has at least two choices. It could call the function directly and keep the current context, or it could take a snapshot during construction and call f in that context. How can the author of Foo know which is appropriate? The answer doesn't need to be the same, f could require the constructor snapshot, and g could require the context that bar was called in. More problematically, bar might need to call a function that sometimes calls f and sometimes calls g.

This isn't a purely abstract concern, it came up while investigating using AsyncContext to power two APIs that are somewhat context-shaped. One is a DI system that flows down the tree of constructors, and the other is an event providence system (to answer questions like "what user interaction prompted this work"). Problems arise because a method on a class doesn't know when a function that it's calling may need to construct an object that participates in the DI system, so it's tempting to just run all of the class's work inside a snapshot taken at construction, but if it does that, then no other context can flow through the class, so work can't be reliably attributed to user interactions.

Taking a step back, it seems like this proposal introduces a function coloring problem – as described in What color is your function? – where a function's color is determined by its current call stack and the snapshots it has access to, there are N different colors, and there are few constraints on the relation defining which colors can call which. This information can't be known statically, and so there's limited ability for documentation or tooling to help. Taken together, I don't see how to build composable and reliable abstractions with AsyncContext.

@Qard
Copy link

Qard commented Jul 29, 2024

It'd just run within whatever context it's already in. Context is based on causality, which means a call should always inherit the context its already in. For the case of a snapshot restore, the context would still flow all the way up to that point, it just gets replaced whenever a specific decision has been made to inherit a different context than the most directly received one.

@rictic
Copy link
Author

rictic commented Jul 30, 2024

@Qard I'm not sure which part of my post that's in response to.

@Qard
Copy link

Qard commented Jul 30, 2024

When we call f and g in bar(), this class has at least two choices. It could call the function directly and keep the current context, or it could take a snapshot during construction and call f in that context.

It would always using the current context unless the user code explicitly decides to bind the context differently somehow. It's not a matter of picking one or the other, the current context is selected by default and should be assumed to be the correct context in almost all cases.

@rictic
Copy link
Author

rictic commented Jul 30, 2024

Why is that correct? By convention?

People can write code with AsyncContext that does require snapshotting and restoring in the constructor (I've seen a real world dependency injection system with roughly this shape). It would even work reasonably well in isolation, until there was a second protocol in the codebase that used AsyncContext and needed context to flow down through methods. This is my concern about composition and function coloring, where it's possible to design locally valid protocols that can't be composed, and you can get into a situation where some functions can't call others, because they have mutually incompatible context requirements.

@Jamesernator
Copy link

This is my concern about composition and function coloring, where it's possible to design locally valid protocols that can't be composed,

This is true of a lot of things. I imagine some people will try the pattern outlined in the original post and hit problems, but preventing all anti-patterns is pretty much impossible.

If people want to use AsyncContext for dependency injection use cases, they are probably better off just not snapshotting in constructors. This is more flexible anyway as they can configure dependencies per (method) call if desired.

@rictic
Copy link
Author

rictic commented Jul 30, 2024

This is true of a lot of things.

What else are you thinking of here?

I imagine some people will try the pattern outlined in the original post and hit problems

They won't hit problems until there's a second AsyncContext protocol in use in the codebase that uses a different convention. By that point they might be in a real bad way, if two or more mutually incompatible protocols developed separately and got fairly big before someone needed to compose them.

Apps will have context information available in some call stacks but not others, and the obvious solution in the API is to create a protocol of "You must create a snapshot in [this condition] and then run [other work] inside that snapshot." And it's not obvious that doing so introduces the function coloring problem where code that adheres to one such protocol can't call or be called by code that adheres to a different protocol.

@dfahlander
Copy link

I would argue that a DI systems should work with a dedicated AsyncContext.Variable instead. A DI system will probably store its own mapping model within one AsyncContext.Variable. Snapshotting is only meant for job schedulers that are unaware and uninterested of variables.

@Jamesernator
Copy link

Jamesernator commented Jul 30, 2024

What else are you thinking of here?

Basically every anti-pattern ever, just because something is an anti-pattern doesn't mean developers won't write things that way anyway. (Often because they don't even realise what they're doing is an anti-pattern).

AsyncContext is specifically designed for the usage pattern where AsyncContext.Snapshot is used to associate a async state to a callback (see also web integration), if people use it other ways I really do feel like that's their problem.


Also generally speaking, I would say capturing the context at constructor time is an anti-pattern as you're effectively taking a dependency on some arbitrary global state.

Like consider this example:

const logger = new AsyncContext.Variable({ default: defaultLogger });

function doSomething() {
    logger.get().log("Some message");
}

class Service {
    #snapshot = new AsyncContext.Snapshot();
    
    method() {
        this.#snapshot.run(() => {
             doSomething();
        });
    }
}

by capturing a snapshot here, the Service class has essentially taken a transitive dependency on logger even though it doesn't use logger, doSomething is the thing that has a dependency on logger not Service.

This has big anti-pattern vibes, why is Service taking a dependency on things it doesn't need? If it needs a specific thing it could just get that thing from a variable (not snapshot):

class Service {
    // Gotten once at construction time, if this is the right time to do so
    // though I would argue it's more flexible for *most* dependencies
    // to simply get them when they are actually used
    #dependency = dependencyVar.get();
}

@rictic
Copy link
Author

rictic commented Jul 30, 2024

The constructor use case came from a project that was aiming to have a tree of providers and consumers, somewhat similar to React context, where any consumer gets its value from the closest providing ancestor, and that ancestor is determined by tree of which objects construct which.

I agree that a leaf node would just grab any variables they need at construction time:

class Service {
    #dependency = validateDependency(dependencyVar.get());
}

And everything is easy when classes construct their children during their own constructors (which is how the app worked initially).

The problems arise when they do construction at any other time. So a change comes in:

 class Requester {
-  #service = new Service();
+  // Service is heavy weight, improve page load time by initializing it on first use.
+  #service = null;

  makeRequest() {
+    if (this.#service === null) {
+      // depending on who first called makeRequest, this may throw because dependencyVar is invalid
+      this.#service = new Service(); 
+    }
     const response = this.#service.actuallyMakeTheRequest();
     // yada yada yada
     return response;
   }
 }

Neither Requester nor the caller to makeRequest knows or cares about the correct value of dependencyVar, that flows down from the object that creates a Requester (or the object that made it, etc).

So to fix it, the author of this change will probably create a snapshot in Requester's constructor and run new Service() in it. And that would work! Until some other code running in the Service constructor has different needs for context to flow down.

@rictic
Copy link
Author

rictic commented Jul 30, 2024

You could work around this by doing, essentially a local implementation of AsyncContext where all of the system's state is stored within a single variable so that it composes. Something like:

const diInfo = new AsyncContext.Variable({});

export class DiVariable<T> {
  #key = Symbol();
  get(): T | null {
    return diInfo.get()[this.#key];
  }
  run(value: T, f: () => void) {
    const currentVal = diInfo.get();
    diInfo.run({ ...currentVal, [this.#key]: value }, f);
  }
}

export class DiSnapshot {
  #diVal = diInfo.get();

  run(f: () => void) {
    diInfo.run(this.#diVal, f);
  }
}

That way you're wrapping the entire DI system's state into a single variable, so Requester can store and restore a DiSnapshot without interfering with other protocols built on top of AsyncContext.

@jridgewell
Copy link
Member

jridgewell commented Jul 31, 2024

The core premise of function coloring is that red (async) functions can call red functions and blue (sync) functions, and blue functions can only call blue functions. In order for a blue function to call a red function, we have to change the blue function into a red one. Then all the callers of our now-red function also need to be changed. So red functions are viral.

You could argue that parameters to functions causes a viral color change. If function foo calls bar which calls baz, and baz requires an explicit param, then that has to passed down each step. baz has to receive it, so bar has to receive it, and foo has to pass it. The param forces each call to change.

If we change the explicit param to an implicit param with AsyncContext, we haven't forced a color change, we've only changed the way we pass the data down. It could be an explicit regular param, or an implicit param via AC if we're within a callstack. If there's not a direct callstack hierarchy, then this requires an explicit Snapshot param to be passed down. But this is all the same. AC doesn't color the function, changing the function to require that param colors it.

I haven't seen how your current DI system currently works, so I can't comment how it works now. But I imagine it works similarly to what's described above, requiring some state to be passed into the services helper.

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

5 participants