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

static vs per instance snapshot #85

Open
Flarna opened this issue May 2, 2024 · 5 comments
Open

static vs per instance snapshot #85

Flarna opened this issue May 2, 2024 · 5 comments

Comments

@Flarna
Copy link

Flarna commented May 2, 2024

In current proposal snapshot() is a static method capturing full context of all AsyncContext variables.
As a result snapshot.run() impacts all AsyncContext users.

This differs from asyncVar.run() usage where current context entries for other asyncVar instances are not impacted.

This global context restore sounds reasonable for library authors (or runtime internally) which knows and defines the global context flow. For example restoring JS context after interacting with some scheduling outside of JS (e.g. interacting with OS sockets,...).

But with the discussions ongoing in #83 or AsyncContext.callingContext() I'm not that sure if this global snapshot/restore is a good thing in general.

Once some AsyncContext user starts to call snapshot() to customize/tune it's context flow it might break other users.

Not sure a an instance snapshot() API is the way to go. Maybe it is more a doc topic to tell users to prefer const myData = asyncVar.get() and asyncVar.run(myData) instead snapshot().

@Qard
Copy link

Qard commented May 2, 2024

I think static snapshot is fine, we just need per-instance escape routes. I think we should have a recommended default path which is usable in most cases, we just would want some tools to reach the other paths in the few cases which don't quite fit. Ideally though the need for manual work should be kept to a minimum through reasonable defaults.

@jridgewell
Copy link
Member

I agree, especially if we go with call-time context being propagated by default, that the global snapshot will likely lead to broken behavior. Shay is discussing a "delimited continuation" idea what might work better. Instead of capturing all variables globally, it captures all variables up until the last time my variable was set:

const mine = new Variable();
const outer = new Variable();
const childOther = new Variable();

const snap = outer.run(1, () => {
  // outer is outside the last `mine.run()`, so it won't be captured.

  return mine.run('abc', () => {
    // child other is inside the last `mine.run()`, so it is captured.
    return childOther.run(true, () => {
      return Snapshot.partial(mine);
    });
  });
});

snap.run(() => {
  assert.equal(outer.get(), undefined);
  assert.equal(mine.get(), 1);
  assert.equal(childOther.get(), true);
});

@Flarna
Copy link
Author

Flarna commented May 2, 2024

I don't think that the nesting should matter here. Either the variables are independent or they aren't.
Consider other and mine are context variables created by independent tools using some sort of instrumentation/hook instrumentation at the same place because both are interested in it for their own special and private thing.
The sequence of the run calls only depends in the init sequence of these tools. This is an unwanted hidden binding in my opinion.

I see value for a global snapshot and for a per variable snapshot:

  • global is useful for interaction with the outside (e.g. OS)
  • private is useful to tune propagation for my needs

@shicks
Copy link

shicks commented May 11, 2024

What would a per-variable (instance) snapshot look like? Isn't it just myVar.get()/myVar.run()?

const snapshot = myVar.get();
// ...
myVar.run(snapshot, () => ...);

Overall, I don't really understand the point of any solution that doesn't handle all variables implicitly by default. The whole point of a "context" is to remove the need for intervening layers to have any knowledge of the data that's passing through them - the moment you add a requirement to handle any variables explicitly, they're no longer propagated through the intervening code that doesn't (or can't) know about them.

@Flarna
Copy link
Author

Flarna commented May 13, 2024

Yes, it's myVar.get()/myVar.run().
Main point is if a dedicated API for this should be visible in the AsyncContext API surface to avoid that people go for global even if they shouldn't.

The global handing is nice for library authors to express how context flows through their scheduler,...

Local handling is needed for users which do not agree to this for whatever reason.
Or if some 3rd party tooling (e.g. OTel instrumentations) "adds" context handling via monkey patching because a library hasn't implemented it yet/correct. Such tools usually do not want to create global side effects.

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

4 participants