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

Add mutation explainer #101

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add mutation explainer #101

wants to merge 1 commit into from

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented Jul 24, 2024

Compare run and set semantic (@@enter and @@dispose), and an example that could potentially be implemented on AsyncContext with set semantic:

async function doWork() {
  using parent = tracer.startAsCurrentSpan("parent");
  // do some work that 'parent' tracks
  console.log("doing some work...");
  // Create a nested span to track nested work
  {
    using child = tracer.startAsCurrentSpan("child");
    // do some work that 'child' tracks
    console.log("doing some nested work...")
    // the nested span is closed when it's out of scope
  }
  // This parent span is also closed when it goes out of scope
}

@legendecas legendecas force-pushed the context-scope branch 4 times, most recently from ee98862 to 9af8820 Compare July 24, 2024 16:38
Copy link

@Qard Qard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This mostly matches my thinking on the subject, though there's some nuance missing in how scopes apply within inner functions:

  • Does the value remain the same in nested sync calls?
  • Does the scope propagate as context does so async continuations can also set values that feed forward from that point in the context?

By providing implicit scopes we would mostly eliminate the need for user-provided scopes and could simply forward-propagate the values of any set calls to subsequent execution. It also flows much more naturally into flow-through semantics and provides for a more friendly interface to supporting multiple flow styles as you no longer need separate variables, you can just have separate setters that specify the flow for the value given at that point. (Actually, you can do the same with .run(...) too, but the semantics are more awkward given how the scoping of it suggests that values should not flow out.

SYNC-MUTATION.md Outdated
```js
const asyncVar = new AsyncContext.Variable({ defaultValue: "default" });

asyncVar.set("A"); // Throws ReferenceError: Not in a mutable context scope.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My expectation would be that top-level would have an implicit scope so there should never be a point where store.set(...) can throw. This is also a requirement for solving the top-level configuration sharing problem Matteo described.

Rather, I would want for this AsyncContext.contextScope(...) thing to be an option for a user to provide additional scopes when they want ones more narrow than the runtime already provides. The runtime-provided scopes I'm thinking of would be module/script top-level and async functions. I believe everything else could just inherit the scope as it inherits context.

SYNC-MUTATION.md Outdated
Comment on lines 241 to 298
This will need a return-value API to feedback the final context snapshot to the
next function paragraph.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sort of scenario is why I keep bringing up flow-through semantics. If context flows out of the return of that async function into subsequent execution then this interface is no longer needed.

SYNC-MUTATION.md Outdated
The most important trait of `set` is that it will not mutate existing
`AsyncContext.Snapshot`.

A userland polyfill like the following one can not preserve this trait.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Layering over unaltered AsyncContext would certainly not work, but patching snapshot retrieval can do this.

@legendecas legendecas marked this pull request as ready for review July 29, 2024 10:35
@legendecas legendecas force-pushed the context-scope branch 2 times, most recently from 3d5b045 to 62d91a0 Compare July 29, 2024 11:04
To preserve the strong scope guarantees provided by `run`, an additional
constraint can also be put to `set` to declare explicit scopes of mutation.

A dedicated `AsyncContext.contextScope` can be decoupled with `run` to open a
Copy link

@Flarna Flarna Jul 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see the benefit of a contextScope.run(). If one is willing to add a new function for scoping the existing run should be ok already.
I'm also not sure if nesting of contextScopes across works as expected if it crosses library boundaries where the inner library might call back to the caller.
To my understanding a contextScope effects all variables so adding an inner scope might break a mutation for the other.

function foo(cb) {
  AsyncContext.contextScope(() => {
    // do some stuff
   cb(); // call the callback which mutates asyncVar
   // do some other stuff
  });
}

function bar() {
  AsyncContext.contextScope(() => {
    asyncVar.set("1");
    foo(() => { asyncVar.set("2") };  // this mutation would be "lost"
    asyncVar.get() // "1"
  });
}

> Note: this also applies to [`ContinuationVariable`][]

However, the well-known symbol `@@dispose` and `@@enter` is not bound to the
`using` declaration syntax, and they can be invoked manually. This can be a
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Allowing a @@enter without @@dispose opens the door for leaks. Either by forgetting @@dispose or calling them in the wrong order.
In my opinion the major benefit of a scoped set is gone because of this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@enter and @@dispose are expected to be used with using declaration. Allowing it to be invoked manually is primarily for library extension, and shown in example below that compositing active spans with the context variables.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as they are used correct in objects which are again intended for use with using and as a result the correct nesting is kept it should be fine.
If it is easy to mix up sequence of @@dispose it's quite problematic.

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

Successfully merging this pull request may close these issues.

3 participants