Specifying (async) generator behavior

[legendecas]

Related: nodejs/node#42237, open-telemetry/opentelemetry-js#2951

Should a generator or an async generator capture the async context when being created? Given:

const context = new AsyncContext();

async function* gen(context) {
  await Promise.resolve();
  // ???
  console.log(context.get());
  yield;
  await Promise.resolve();
  // ???
  console.log(context.get());
}

let g;
context.run('init', () => {
  g = gen(context);
});
await context.run('first iter', async () => {
  await g.next();
});
await context.run('second iter', async () => {
  await g.next();
});

The output options could be:

1. 'first iter'/'second iter', which is the context when the generator's next() is invoked.
2. 'init'/'init', which is the context when the generator is being constructed.

[jasnell]

My initial instinct is option 1, first iter / second iter... specifically, the context whenever next() is invoked but I do see cases where option 2 would be useful also. Unfortunately, with the current AsyncLocalStorage API option 2 is very cumbersome to implement.

const { AsyncLocalStorage, AsyncResource } = require('async_hooks');

const als = new AsyncLocalStorage();

function* gen(resource) {
  yield resource.runInAsyncScope(() => als.getStore());
  yield resource.runInAsyncScope(() => als.getStore());
}

let g = als.run(123, () => gen(new AsyncResource("abc")));

console.log(g.next());

I'd suggest that in the default case, it should be option 1 but we should have something defined that allows for option 2.

[jridgewell]

I think Option 1 is the correct choice, because it matches the sync calls-stack behavior that AsyncContext is based on.

I'd suggest that in the default case, it should be option 1 but we should have something defined that allows for option 2.

It's not too hard to define a wrapper that does this without you having to change your generator function:

const it = wrapGen(gen());
it.next();
it.next();

function wrapGen(it) {
  const getNext = AsyncContext.wrap(() => it.next());
  if (it[Symbol.iterator]) {
    return wrapSync(it, getNext);
  }
  return wrapAsync(it, getNext);
}

function* wrapSync(getNext) {
  while (true) {
    const next = getNext();
    if (next.done) return next;
    yield next;
  }
}
async function* wrapAsync(getNext) {
  while (true) {
    const next = await getNext();
    if (next.done) return next;
    yield next;
  }
}

[jridgewell]

Actually, we'll need a bit of spec test to handle parallel calls to an async generator:

const ctx = new AsyncContext();

async function* foo() {
  // Make it so the gen doesn't complete sync
  await sleep(100);

  console.log(ctx.get());

  yield;

  console.log(ctx.get());
}

const it = ctx.run(1, () => foo());

await ctx.run(2, () => it.next());
await ctx.run(3, () => it.next());

Which combination of 1, 2, and 3 should this log?:

1. 1, 1
2. 1, 2
3. 1, 3
4. 2, 2
5. 2, 3
6. 3, 3 Obviously not this one, right?

Well, currently it logs 2, 3. How about this setup:

const it = ctx.run(1, () => foo());

await Promise.all([
  ctx.run(2, () => it.next()),
  ctx.run(3, () => it.next()),
]);

This one logs 2, 2!

The queuing behavior of async generators is a little weird. It maintains its own queue of promises, but doesn't use HostCreateJobCallback. And, if there are multiple parallel calls, it will continue processing the queue.

[jridgewell]

During today's meeting, we're aligned with having generators record the context at the initialization time, and having every .next() just restore the initialization time's context. So the answer to https://github.com/tc39/proposal-async-context/issues/18#issuecomment-1463304093 is 1, 1 (the first choice).

[mhofman]

To clarify, this would apply to both generators and async generators, or only the latter?

[jridgewell]

Both

[andreubotella]

While working on the spec text for this, I noticed there are various spec-created iterators which use the generators machinery under the hood through CreateIteratorFromClosure, and would therefore propagate the async snapshot from the time the iterator is created.

This can only be observed if there's some user code running inside the generator, but in the spec as it is right now, that is already the case for array iterators (and I think also regexp iterators) if some property has a getter:

const asyncVar = new AsyncContext.Variable();

const array = [23, 34, 45];
Object.defineProperty(array, 1, {
  get() {
    return asyncVar.get();
  }
});

const iter = asyncVar.run("foo", () => array.values());

array[0] = 12;
asyncVar.run("bar", () => {
    console.log([...iter]);  // [12, "foo", 45]
});

And with the iterator helper proposal, any iterator helper callbacks would run in the generator context:

const asyncVar = new AsyncContext.Variable();

const array = [23, 34, 45];
const iter1 = array.values();
const iter2 = asyncVar.run("foo", () => {
    return iter1.map(v => asyncVar.get());
});
asyncVar.run("bar", () => {
    console.log(iter2.next());  // { value: "foo", done: false }
});

Is this expected? Or should we somehow exclude CreateIteratorFromClosure from this behavior?

[jridgewell]

Would it be difficult to exclude? I don't think the behavior is problematic, but I would prefer consistency for the language's iterators.

[andreubotella]

For sync iterators, it would need a flag on the generator so it doesn't switch to the snapshot active when the execution was last suspended. For async iterators, I don't know what would be needed yet.

[mhofman]

I would prefer consistency for the language's iterators

Wouldn't the alternative be to make language iterators that don't use internal generators to manually carry the async context?